Problem/Motivation

Drupal\content_moderation\EntityOperations::updateOrCreateFromEntity() has this code in it:

      // If a new revision of the content has been created, add a new content
      // moderation state revision.
      $content_moderation_state->setNewRevision(TRUE);

Which is great, except it results in the newly created moderation state revision being created as a default revision even if the moderated entity is being saved as a Draft. I don't know if this has problems in and of itself, but #2860097: Ensure that content translations can be moderated independently requires the moderation state entity's pending/default status to match the moderated entity's one, and it's possible that this mismatch is also all of or part of the problem that leads to #2933099: "Default Revision cannot be deleted." But it's not the default revision....

To reproduce the bug:

  1. Save an Article as Published. Look at the `content_moderation_state` db table and notice that its revision ID is the latest one from `content_moderation_state_revision`.
  2. Edit the Article and save it as Draft. Look at the `content_moderation_state_revision` db table to confirm a new revision got made. Look at the `content_moderation_state` table. Expected behavior: it should still be the one from step 1, if we want the new revision treated as a pending revision. Actual behavior: It's the latest one, which resulting as the new moderation state revision being treated as a default revision despite the corresponding Article revision still being a pending revision.

Proposed resolution

See patch.

Remaining tasks

Implement hook_update_N() to update existing moderation state entities/revisions to match their host entity's/revision's pending/default status.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#25 cm-ms_default_revision-2941736-25.interdiff.txt1.5 KBplach
#25 cm-ms_default_revision-2941736-25.patch119.93 KBplach
#22 cm-ms_default_revision-2941736-21.patch119.89 KBplach
#22 cm-ms_default_revision-2941736-21.interdiff.txt9.01 KBplach
#18 cm-ms_default_revision-2941736-17.code_.patch16.94 KBplach
#17 cm-ms_default_revision-2941736-17.patch122.25 KBplach
#15 cm-ms_default_revision-2941736-15.patch265.11 KBplach
#15 cm-ms_default_revision-2941736-15.interdiff.txt6.11 KBplach
#13 cm-ms_default_revision-2941736-13.patch262.08 KBSam152
#13 cm-ms_default_revision-2941736-13.interdiff.txt251.83 KBSam152
#13 2941736-create-content-script.txt1.8 KBSam152
#12 interdiff-fail.txt88.34 KBSam152
#10 cm-ms_default_revision-2941736-10.patch10.25 KBplach
#10 cm-ms_default_revision-2941736-10.interdiff.txt4.22 KBplach
#7 cm-ms_default_revision-2941736-7.patch7.83 KBplach
#7 cm-ms_default_revision-2941736-7.interdiff.txt4.39 KBplach
#4 interdiff.txt1010 byteseffulgentsia
#4 cm-pending-4.patch3.44 KBeffulgentsia
#2 cm-pending.patch2.28 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
2.28 KB

Status: Needs review » Needs work

The last submitted patch, 2: cm-pending.patch, failed testing. View results

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
1010 bytes

Copying the ViewsDataIntegrationTest changes from #2860097: Ensure that content translations can be moderated independently.

effulgentsia’s picture

  1. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsDataIntegrationTest.php
    @@ -108,14 +108,12 @@ public function testContentModerationStateBaseJoin() {
    -        // @todo I would have expected that the content_moderation_state default
    -        //   revision is the same one as in the node, but it isn't.
    

    Yay that this patch fixes a WTF that already tripped up whoever wrote that :)

  2. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsDataIntegrationTest.php
    @@ -108,14 +108,12 @@ public function testContentModerationStateBaseJoin() {
             // Joins from the revision table to the revision of the
             // content_moderation.
             'moderation_state_2' => 'published',
    

    Seems like we're now missing test coverage for some case where there's 'draft' as a value?

plach’s picture

Assigned: Unassigned » plach

Working on the update logic

plach’s picture

This implements the upgrade path, we need test coverage for both the upgrade and the fix itself. I'll work on that over the weekend if none beats me to it.

Sam152’s picture

Status: Needs review » Needs work

The last submitted patch, 7: cm-ms_default_revision-2941736-7.patch, failed testing. View results

plach’s picture

Here's a test for the main functionality, we are only missing the update test now.

plach’s picture

Status: Needs work » Needs review
Sam152’s picture

FileSize
88.34 KB

I had a go at creating some db fixtures based on drupal-8.bare.standard.php.gz which would install and update core, install content_moderation and any associated content at the 8.4.0 schema, so there is a general foundation for update path tests at the point which content moderation hit stable. This approach doesn't work very well, because there is a mixture of rows from the 8.0.0 schema and 8.4.0 schema. New columns introduced between 8.0 and 8.4 are mismatched before you call runUpdates in the test. It's also probably a bad idea to try and create tests that aren't based on completely concrete snapshots of the schemas we are trying to test against.

I know there is always hesitation to introduce new full gzipped snapshots, but I think based on cm being new in 8.4.0, we need it. For that reason I think it makes sense to create some new fixtures for this:

  • drupal-8.4.0.bare.standard.php.gz: Installation of standard at 8.4.0, can be used for other update path test with things introduced for the first time in 8.4.
  • drupal-8.4.0-content_moderation_installed.php: Installing content moderation on top of 8.4.0, can be used for other CM update test.
  • 2941736-content.php: Content created at 8.4.0 required to test this issue.

Happy to have a squiz at doing this, or if you have a plan for it @plach, I can leave you to it. I've attached an interdiff on top of #10 which is a rough failed attempt at using drupal-8.bare.standard.php.gz.

Sam152’s picture

Here is an implementation of the fixtures proposed in #12 and an associated test. If the test cases change, I've attached the script used to generate content for the third fixture file referenced by the test. The first two can be imported, the script can be run and the resulting delta is just the test cases required for the test.

Apologies if this is stepping on the toes of any work in progress or plans you had for the issue, if it is, feel free to go in another direction.

plach’s picture

Assigned: Unassigned » plach

Looks great, however while testing it I realized there was a problem with the update code. Working on a fix.

plach’s picture

This seems to work nicely now. I'm attaching also a before/after SQL snapshot of the database generated by Sam, for people wishing to review its content.

effulgentsia’s picture

I split out #2942073: Add a "bare.standard.php.gz" test fixture that starts from Drupal 8.4.0., RTBC'd it, and pinged several of the other committers about it to get their take on it.

plach’s picture

plach’s picture

Attaching also a code-only patch, for reviewers' convenience.

timmillwood’s picture

  1. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,178 @@
    +  /** @var \Drupal\Core\Entity\Sql\SqlContentEntityStorage $cms_storage */
    +  $cms_storage = $entity_type_manager->getStorage('content_moderation_state');
    

    We don't generally abbreviate things, and cms means more than Content Moderation State.

  2. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,178 @@
    +    ->condition($entity_type->getKey('bundle'), $bundles, 'IN')
    

    When happens if the entity type doesn't have a bundle key?

  3. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,178 @@
    +    FROM {{$cms_storage->getRevisionDataTable()}} cms
    

    Maybe we should set the alias to cmsfr, because that's the revision data table?

  4. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
    @@ -536,6 +536,44 @@ public function testWorkflowNonConfigBundleDependencies() {
    +    $entity_type_id = 'entity_test_mulrevpub';
    

    It might be good to test other entity types too? Such as entity_test_no_bundle.

amateescu’s picture

  1. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,178 @@
    +function content_moderation_post_update_update_cms_default_revisions(&$sandbox) {
    

    I think this update function is unnecessarily complex.

    First, it only deals with SQL storages without a check whether the moderated entity is using a SQL storage or not.

    Second, I don't really understand why it has to be hardcoded to SQL queries at all. Since it is a post_update function, can't we simply do something like this:

    foreach moderated entity type:
    - do a range entity query on the default revisions
    - get a list of CMS revisions that point to the revision IDs obtained above
    - do a loadMultipleRevisions() on the CMS entities
    - call isDefaultRevision(TRUE); setNewRevision(FALSE); save the CMS revision
    
  2. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -159,11 +160,6 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
    -      $content_moderation_state->setNewRevision(TRUE);
    
    @@ -176,6 +172,12 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
    +      $content_moderation_state = $storage->createRevision($content_moderation_state, $entity->isDefaultRevision());
    

    Is there any reason to use the new createRevision() method instead of the previous setNewRevision(TRUE) call? Do we need to merge the default revision with the active translation of the CMS entity?

Sam152’s picture

Adding an issue for a fixture to enable entity_test for scenarios like #19.4.

plach’s picture

plach’s picture

@amateescu, #20.2:

Well, aside from the fact that that's the new recommended way to instantiate new revisions (we may even add a hook_entity_revision_create() invocation to that method), yes, we will be going to need the multilingual merge logic in #2860097: Ensure that content translations can be moderated independently.

amateescu’s picture

Nice, this looks much cleaner now! Just a few nitpicks from me left:

  1. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,94 @@
    +  // For every moderated entity identify the default revision ID, track the
    

    Needs a comma between "entity" and "identify" :)

  2. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,94 @@
    +  // revision in the "content_moderation_state" entity base table.
    

    We don't do direct table queries anymore, so I think we can remove the "in the "content_moderation_state" entity base table" part of this comment.

  3. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,94 @@
    +  $entity_type_id = &$sandbox['entity_type_id'];
    

    Are we sure this doesn't throw a notice the first the update function runs?

  4. +++ b/core/modules/content_moderation/content_moderation.post_update.php
    @@ -0,0 +1,94 @@
    +    $sandbox['limit'] = 10;
    

    We could use $settings['entity_update_batch_size'] for the default limit, 10 seems to be a very low limit.

plach’s picture

@amateescu:

Are we sure this doesn't throw a notice the first the update function runs?

Yep, the & operator initializes the key as NULL if it's missing.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Cool, I think this is ready now :)

effulgentsia’s picture

Looks great. Adding reviewer credit.

  • effulgentsia committed 7f1193a on 8.6.x
    Issue #2941736 by plach, Sam152, amateescu, timmillwood: Moderation...

  • effulgentsia committed 42baaa6 on 8.5.x
    Issue #2941736 by plach, Sam152, amateescu, timmillwood: Moderation...
effulgentsia’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Pushed to 8.6.x and 8.5.x.

effulgentsia’s picture

Title: Moderation state revisions should have their isDefaultRevision() match the host entity's » [Needs change record] Moderation state revisions should have their isDefaultRevision() match the host entity's
Status: Fixed » Needs work
Issue tags: +Needs change record, +Needs followup

Come to think of it, I think this could use a change record, just in case anyone was relying on the prior behavior of the content_moderation_state entity's default revision always being the latest one regardless of the state of the moderated entity.

Also, while #19.2 got fixed, it doesn't look like the entity-type-with-no-bundle case was added to the test fixture. Could we have a followup to do so, just to make sure that code path has test coverage?

effulgentsia’s picture

just in case anyone was relying on the prior behavior

Note that content_moderation was still experimental in 8.4. So that makes me less clear on whether a change record for this is needed or not. What do others think?

plach’s picture

@effulgentsia:

Also, while #19.2 got fixed, it doesn't look like the entity-type-with-no-bundle case was added to the test fixture. Could we have a followup to do so, just to make sure that code path has test coverage?

We had opened #2942196: Create a database fixture to enable entity_test on top of drupal-8.4.0.bare.standard., but it seems entity_test is not a good candidate for a test fixture.


I'm not sure about the CR: aside from the fact that CM was experimental so far, the CMS entity type is internal and not supposed to be interacted with directly. Of course there is a behavior change, but I'm not sure whether this change is perceivable in any other way then a bug fix. I'd like to read the CM maintainers' opinions on this.

plach’s picture

Title: [Needs change record] Moderation state revisions should have their isDefaultRevision() match the host entity's » Moderation state revisions should have their isDefaultRevision() match the host entity's
Status: Needs work » Fixed
Issue tags: -Needs change record, -Needs followup

Tentatively marking this as fixed. Feel free to reopen if you disagree.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.