Problem/Motivation

When working on #2766957: Forward revisions + translation UI can result in forked draft revisions I found that a revision was getting set as default when it shouldn't be.

This seems unrelated to the original issue, because it is caused by content moderation, rather than being exposed by content moderation. The issue also happens without the patch from the other issue.

Proposed resolution

Fix \Drupal\content_moderation\EntityOperations::entityPresave

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 2862988-18.patch6.8 KBtimmillwood
#18 interdiff-2862988-18.txt3.11 KBtimmillwood
#14 2862988-14.patch5.71 KBtimmillwood
#14 interdiff-2862988-14.txt1.27 KBtimmillwood
#10 2862988-10.patch5.29 KBtimmillwood
#10 2862988-10-test-only.patch3.45 KBtimmillwood
#10 interdiff-2862988.txt998 bytestimmillwood
#7 2862988-7.patch5.16 KBtimmillwood
#7 2862988-7-test-only.patch3.32 KBtimmillwood
#3 2862988-3.patch5.62 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Issue summary: View changes
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2862988-3.patch5.62 KB

I *think* this proves the issue.

I have a feeling a simpler test is needed to make it clearer.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Priority: Normal » Critical

Since this is blocking another critical issue, and it's data-integrity, bumping to critical too. Thanks for splitting this out.

Status: Needs review » Needs work

The last submitted patch, 3: 2862988-3.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Found the cause of the issue!

In \Drupal\content_moderation\EntityOperations::isDefaultRevisionPublished we check if the default revision is has a published state or not. This is used to set a new revision as default if the existing default is not published.

This issue comes when the default revision of a language is not published, but another language of the default revision is published.

I guess the solution is to loop through all languages to see if any language of the default revision has a published state.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
2862988-7-test-only.patch3.32 KB
2862988-7.patch5.16 KB

Added a new simple test for this, and a fix.

The last submitted patch, 7: 2862988-7-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2862988-7.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2862988.txt998 bytes
2862988-10-test-only.patch3.45 KB
2862988-10.patch5.29 KB

oops.

The last submitted patch, 10: 2862988-10-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2862988-10.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Trying to debug the failing test in \Drupal\Tests\content_moderation\Functional\ModerationLocaleTest::testTranslateModeratedContent and it seems like the update is preventing a new translation becoming the default revision because one of the other languages is published.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2862988-14.txt1.27 KB
2862988-14.patch5.71 KB

This should fix the failing tests by always setting a default revision if it's a new translation, however this introduces a new bug, which will be "fixed" by #2766957: Forward revisions + translation UI can result in forked draft revisions so will add a test for it there.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Nice find!

Very quick review of the patch itself:

  1. +++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -262,21 +263,25 @@ public function entityView(array &$build, EntityInterface $entity, EntityViewDis
     // Ensure we are comparing the same translation as the current entity.

    This comment is no longer true. 

  2. +++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -262,21 +263,25 @@ public function entityView(array &$build, EntityInterface $entity, EntityViewDis
-      // If there is no translation, then there is no default revision and is
-      // therefore not published.

    Would be good to adapt this comment rather than remove it.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Just want to clarify the new bug in #14.

Imagine this case

  en  |  fr
1 1   |  x
2 0   |  x

The site has two languages, when adding an entity revision 1 is published in english, but doesn't yet have a french translation. In revision two we add an unpublished forward / draft revision for english, french still doesn't have a revision. After the patch in #14 if we add any french revision (published or unpublished) it will become the default revision. This will copy revision 1 english loosing revision 2. This is the same issue #2766957: Forward revisions + translation UI can result in forked draft revisions is trying to fix, and not really a new issue, but just a new way of causing it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Interesting find!

+++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -108,6 +108,7 @@ public function entityPresave(EntityInterface $entity) {
       // This entity is default if it is new, the default revision, or the
       // default revision is not published.
       $update_default_revision = $entity->isNew()
+        || $entity->isNewTranslation()
         || $current_state->isDefaultRevisionState()
         || !$this->isDefaultRevisionPublished($entity, $workflow);

I guess we should update the documentation here.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
FileSize
interdiff-2862988-18.txt3.11 KB
2862988-18.patch6.8 KB

Fixing #15 and #17. Also tidying up isDefaultRevisionPublished a little.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented
Status: Needs review » Reviewed & tested by the community

Code & test makes sense to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 55e309f to 8.4.x and f5b24ee to 8.3.x. Thanks!

It always feels weird that default revision is not translatable and publishing status is. Nice to have more testing in this very complex area.

  • alexpott committed 55e309f on 8.4.x 
    Issue #2862988 by timmillwood, catch: EntityOperations::entityPresave...

  • alexpott committed f5b24ee on 8.3.x 
    Issue #2862988 by timmillwood, catch: EntityOperations::entityPresave...

Status: Fixed » Closed (fixed)

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