Problem/Motivation

This is a follow up to #2766957: Forward revisions + translation UI can result in forked draft revisions where we implemented a fix to prevent data loss by only allowing one translation of a single entity to have pending revisions at any one time.

Proposed resolution

The general solution was discussed during various IRC meetings between @catch, @hchonov, @timmilwood and @plach. See #2766957-173: Forward revisions + translation UI can result in forked draft revisions for the main outcomes.

To avoid the data loss scenarios addressed in the parent issue, we agreed that, when dealing with pending revisions of multilingual entities, we can only allow one single translation to be changed for every pending revision. This in turn allows to create a new revision being a merge of the default revision with the latest affected revision for the language being edited.

This approach, coupled with always loading the latest affected revision in entity forms and related routes (latest version, translation overview), allows to let users always work on pending revisions without having to worry about the other languages. When a revision is saved as default, the other languages always match what was previously saved in the previous default revision.

Since changes to untranslatable fields affect all translations in a revision, they can be handled in two ways:

  • we either support them only in default revisions, that is no pending revision can contain changes to untranslatable field values;
  • we make those affect only the default translation, that is only pending revision affecting the default translation can contain changes to untranslatable field values.

The former options is compatible with the current core behavior, while the latter is more suitable when Content Moderation is involved and pending revisions are enabled. This logic can be revisited once we support conflict management in core, but for now it should allow to address the most common use cases without imposing too many constraints on editors, that will be able to select one of the two modes in the Content Translation admin UI.

Implementation details

This exploits the new ::createRevision() method to instantiate a revision object encapsulating all the merging logic described above.

Since we cannot enforce the one translation changed per pending revision constraint in general, as core UIs allow to save entities without creating a new revision, this new logic is leveraged by the Content Moderation module to switch the entity object in the entity form prepare phase.

Programmatic workflows are encouraged to adopt the usage of this new method, but are free not to do so, if it does not fit the use cases they are addressing.

Entity converters are updated to load the latest affected revision, instead of the latest revision when the load_latest_revision option is defined.

Remaining tasks

This issue has several dependencies:

User interface changes

The UI blocks and error messages introduced in #2766957: Forward revisions + translation UI can result in forked draft revisions are now removed.

API changes

None

Data model changes

None

CommentFileSizeAuthor
#176 interdiff.txt841 byteseffulgentsia
#176 entity-ml_drafts-2860097-176.patch51.91 KBeffulgentsia
#173 entity-ml_drafts-2860097-173.patch51.94 KBplach
#173 entity-ml_drafts-2860097-173.interdiff.txt2.27 KBplach
#169 entity-ml_drafts-2860097-169.review.patch51.8 KBplach
#169 entity-ml_drafts-2860097-169.patch171.45 KBplach
#157 entity-ml_drafts-2860097-157.patch54.95 KBplach
#157 entity-ml_drafts-2860097-157.interdiff.txt2.18 KBplach
#152 entity-ml_drafts-2860097-152.patch54.95 KBeffulgentsia
#147 entity-ml_drafts-2860097-147.review.patch54.95 KBplach
#147 entity-ml_drafts-2860097-147.patch71.8 KBplach
#147 entity-ml_drafts-2860097-147.interdiff.txt1.99 KBplach
#135 entity-ml_drafts-2860097-135.interdiff.txt5.97 KBplach
#135 entity-ml_drafts-2860097-135.patch62 KBplach
#127 entity-ml_drafts-2860097-127.interdiff.txt2.21 KBplach
#127 entity-ml_drafts-2860097-127.patch58.21 KBplach
#119 entity-ml_drafts-2860097-119.interdiff.txt656 bytesplach
#119 entity-ml_drafts-2860097-119.patch58.2 KBplach
#117 entity-ml_drafts-2860097-117.interdiff.txt5.94 KBplach
#117 entity-ml_drafts-2860097-117.patch58.19 KBplach
#112 entity-ml_drafts-2860097-111.patch57.85 KBeffulgentsia
#109 entity-ml_drafts-2860097-107.interdiff.txt2.51 KBplach
#109 entity-ml_drafts-2860097-107.review.patch57.85 KBplach
#109 entity-ml_drafts-2860097-107.patch79.71 KBplach
#103 entity-ml_drafts-2860097-103.interdiff.txt35.42 KBplach
#103 entity-ml_drafts-2860097-103.patch84.27 KBplach
#96 entity-ml_drafts-2860097-96.interdiff.txt12.07 KBplach
#96 entity-ml_drafts-2860097-96.patch74.28 KBplach
#91 entity-ml_drafts-2860097-91.interdiff.txt39.5 KBplach
#91 entity-ml_drafts-2860097-91.patch72 KBplach
#87 entity-ml_drafts-2860097-87.interdiff.txt11.76 KBplach
#87 entity-ml_drafts-2860097-87.review.patch62.75 KBplach
#87 entity-ml_drafts-2860097-87.patch133.3 KBplach
#78 entity-ml_drafts-2860097-78.review.patch44.95 KBplach
#77 entity-ml_drafts-2860097-77.review.patch46.22 KBplach
#77 entity-ml_drafts-2860097-77.patch142.04 KBplach
#74 entity-ml_drafts-2860097-74.review.patch51.66 KBplach
#74 entity-ml_drafts-2860097-74.patch144.27 KBplach
#73 et-untranslatable_fields_hide-2878556-73.review.patch52.13 KBplach
#73 et-untranslatable_fields_hide-2878556-73.patch123.9 KBplach
#70 entity-ml_drafts-2860097-70.review.patch52.45 KBplach
#67 entity-ml_drafts-2860097-67.interdiff.txt1.49 KBplach
#67 entity-ml_drafts-2860097-67.review.patch62.07 KBplach
#67 entity-ml_drafts-2860097-67.patch128.8 KBplach
#66 entity-ml_drafts-2860097-66.review.patch77.61 KBplach
#9 2860097-9.patch9.61 KBtimmillwood
#15 2860097-TEST_ONLY-decoupled-language-revisions-15.patch3.95 KBSam152
#17 interdiff-2784201-45.patch4.34 KBtimmillwood
#17 combined-2860097-17-2784201-45.patch33.4 KBtimmillwood
#26 2860097-26.patch6.53 KBtimmillwood
#34 interdiff-2860097-33.txt5.75 KBtimmillwood
#34 2860097-33.patch4.69 KBtimmillwood
#36 interdiff-2860097-36.txt8.53 KBtimmillwood
#36 2860097-36.patch10.5 KBtimmillwood
#42 interdiff-2860097-42.txt11.19 KBtimmillwood
#42 2860097-42.patch17.88 KBtimmillwood
#45 interdiff-2860097-45.txt11.26 KBtimmillwood
#45 2860097-45.patch19.56 KBtimmillwood
#61 entity-ml_drafts-2860097-61.patch127.67 KBplach
#61 entity-ml_drafts-2860097-61.review.patch105.55 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

catch’s picture

I thought there was another issue discussing this general problem, but I can't find it.

I did find this old issue dealing with reverts though, which is relevant: #2453153: Node revisions cannot be reverted per translation.

timmillwood’s picture

I thought #2766957: Forward revisions + translation UI can result in forked draft revisions was the original issue, but it kinda mutated.

timmillwood’s picture

It looks as though this could be a pretty complex change to try and introduce, but possible. If #2721313: Upgrade path between revisionable / non-revisionable entities is possible, anything is, right?

My first thought was allowing multiple entries per entity ID to the base table would be the way to go, one per language. I'm not sure this is the best option, both for performance, but also to keep the sense that one entity is one thing, with all the languages.

@catch mentioned once upon a time about flagging which revisions are or have been the default. I wonder if this is a great use case for that. So in the revision table or revision data table we will have a new column default, this will be relate to a new base field (and maybe a revision_metadata_key). Then when a revision is saved as the default revision the field will get a 1, otherwise a 0.

Then I am thinking the current default revision would be the entity with the highest revision ID marked as default.

This made me think when you do Node::load(1) it gets all languages of the default revision for node 1, if we have different default revisions per language should Node::load(1) get the default revision for the default language (or active?), then the same revision for other languages, or the default revision for all languages?

It seems \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables and \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables is where all the loading of entity data is done, so we'd have to rewrite this to make sure the "correct" data is loaded.

Saving revisions would work in pretty much the same way we do now. Although we could possibly remove the copying of revision translations. Maybe going forward each revision should only have one language? Although if you called $entity->getTranslation('fr'); on a revision (not the latest default) which french revision would it get?

Still so many questions, but I think the first step is adding a default field to \Drupal\Core\Entity\RevisionLogEntityTrait::revisionLogBaseFieldDefinitions.

tacituseu’s picture

How about allowing there to be only one forward/draft/scratchpad revision per given nid/langcode combination and store it in node_field_revision always in fixed location, say under vid = CONSTANT_OF_FORWARD_REVISION = 0.

When editing translation, check first if there is anything under vid = 0 for that langcode and use it as base if there is, otherwise use the latest revision for that language.

When saving/publishing the translation based on vid = 0 the thing becomes latest/default revision for this langcode, vid = 0 for this langcode can be deleted, and all the other languages get their last non-vid-0 revision copied (even when they have something under vid = 0) for the new default.

When saving/leaving unpublished "just" store it back under vid = 0.

Sidenote: having different default revision per langcode would make using entity_reference_revisions really painful.

timmillwood’s picture

One forward revision per language is pretty much what we're doing in #2766957: Forward revisions + translation UI can result in forked draft revisions but only enforcing it at a UI level. It has too many restrictions when working with complex sites where different teams may be working on each language and need to create one or more forward revision per language at a time.

Interesting point you make about entity_reference_revisions, but as long as the entity reference revision field is translatable it shouldn't be an issue.

tacituseu’s picture

@timmillwood: #2766957: Forward revisions + translation UI can result in forked draft revisions looks like one forward revision per nid not per nid/langcode combination.

timmillwood’s picture

@tacituseu - Ah yes, I see what you mean now.

timmillwood’s picture

Status: Active » Needs review
FileSize
9.61 KB

Initial patch adding a 'default_revision' field which denotes if a revision is or has ever been the default revision.

Also added a setter for it, because using isDefaultRevision() as a setter is crazy.

Basic test for this added to Content Moderation, but I guess we should also test this within the entity api itself.

Status: Needs review » Needs work

The last submitted patch, 9: 2860097-9.patch, failed testing.

catch’s picture

fwiw I think it's unlikely that we can implement this in such a way that the query performance is good with large data-sets - for example entity queries would likely have to do extra conditions and/or joins to figure out what they're querying against depending on language.

So similar to that revision revert patch, it's probably worth looking more at how to combine the 'published' language version from two different revisions when saving. The logic for reverting one language at a time isn't really any different from publishing one language at a time in terms of what happens when saving.

timmillwood’s picture

@catch - I think we can find solutions to the performance issues, and if I understand your suggestion right, I have doubts it'll solve the issue.

timmillwood’s picture

After talking this issue through with @catch yesterday we came to the conclusion that if the latest revision was not always the revision with the highest revision ID then we should be able to solve this.

So I kept working from the patch in #9 where we were flagging which revisions have been the default. However upon further testing this will only work if we go back and update the field if we stem a default revision from it.

For example, if we create and publish a Node (revision 1), then create a forward revision (revision 2). Revision 1 will be the default, and revision 2 isn't. If we then published revision 2 it will create revision 3 which will then be the default. However the default_revision field on revision 2 will not be updated therefore this will still be seen as a draft / forward revision.

Therefore I wonder if #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types has more promise. As it stores the latest revision as a non-revisionable field.

timmillwood’s picture

I've been investigating if #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types holds a solution to the issue, and the answer is kinda.

When we edit an entity with Content Moderation we get / stem the latest revision rather than the default revision as core does by default. We determine the latest revision as the one with the greatest revision id. Although as we know from this issue and #2766957: Forward revisions + translation UI can result in forked draft revisions before it, Content Moderation has the ability of making the "latest revision" have a lower revision id than the default revision.

In this issue we were looking to make entities which has not been default, this would allow us to edit from the latest draft (never been default) revision, this didn't go to plan because when revisions are not default, then made default it creates a new revision, it doesn't update the old revision to be default.

In #2766957: Forward revisions + translation UI can result in forked draft revisions Sam is looking to set a latest revision field. This field is not revisionable or translatable, therefore all revisions and languages denote the same "latest revision". I had a play with the code to set a different latest revision per language. This would allow english and french to have a revision 3 which is default and published, but only for french the latest revision is 3, for english revision 2 is the latest, because it was created as a draft revision. Revision 3 of english is just a copy of revision 1. However if loading the Node edit form with revision 2 then trying to save threw the error The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.. This is because there is a newer revision in one or more languages, and is blocked by EntityChangedContraintValidator incase there are shared fields. When talking this over with Catch we looked at reverting translated revisions, as this works in the same way. Revisions work fine because they don't do validation. The other issue faced with having a different "latest revision" per language is it didn't just happen with draft revisions. Any revision created with cause different latest revisions to be denoted by the field. This would then cause the same validation error.

I think the only real solution would be going back and decoupling language revisions from each other. However there will need to be some kind of link because of shared fields.

Sam152’s picture

I think switching to a TDD approach to decide how the API should behave makes sense. Attached is a patch which articulates the direction I think we're at least aiming for.

If we can agree how this should work, before diving into the murky implementation details, I think that would be helpful.

Sam152’s picture

Status: Needs review » Needs work
timmillwood’s picture

This is a bit of a strange patch, the interdiff is based off patch 45 #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types, and the combined patch is a combination of the interdiff and patch 45 from the other issue.

In this patch we are storing the "latest" revision for each language. Then when editing the entity (creating a new revision) we use that "latest" revision as the starting point, rather than using the default revision (as core does) or the revision with the highest revision id (as Content Moderation currently does).

Although the issue I am having is; Create an unpublished english revision (revision 1), create an unpublished french translation (revision 2), publish the english revision, this will use revision 1 as a starting point and save without a french translation. So there is no french revision 3, therefore /fr/node/1 doesn't exist.

I think we might need to do something like \Drupal\node\Form\NodeRevisionRevertTranslationForm::prepareRevertedRevision where we loop through field definitions and set the field value to the value we want from each translated revisions, but this is getting into rewriting Entity API territory.

The last submitted patch, 17: interdiff-2784201-45.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: combined-2860097-17-2784201-45.patch, failed testing.

hchonov’s picture

Re #13:

For example, if we create and publish a Node (revision 1), then create a forward revision (revision 2). Revision 1 will be the default, and revision 2 isn't. If we then published revision 2 it will create revision 3 which will then be the default. However the default_revision field on revision 2 will not be updated therefore this will still be seen as a draft / forward revision.

That is exactly what I would except to happen. What are your concerns about this? There was one thing that has always bothered me with the forward revision concept - it is impossible to see in the history if a specific revision has been the default one or a draft one.

Also it might happen that user A is working on a draft revision and meanwhile user B publishes a new revision - this does not make the draft revision of user A obsolete, but shows that here we need a conflict management as well in order to perform an automatic merge or let the user A merge the changes from the new default revision into the draft revision. However even without conflict management the draft revision remains a draft revision after a new default revision has been created by the same or a different user.

timmillwood’s picture

@hchonov - I think conflict management will be a follow up, and something the Workflow initiative is planning. We've already written a library https://github.com/relaxedws/merge which will perform a 3-way merge of PHP arrays. This allows us to normalize an entity to an array then merge. For this to work we'll need to properly track parent revisions so we know the correct original revision.

In this issue we are just looking to solve the issue of draft / forward revisions becoming past revisions, and being able to stem from that.

hchonov’s picture

In this issue we are just looking to solve the issue of draft / forward revisions becoming past revisions, and being able to stem from that.

Yes and I think that your example where revision 2 stays a draft one and revision 3 is the default is the right way to go.

@timmillwood, it is nice that you intend working on conflict management, because it is my next step after autosave_form, which is actually ready. I've already sent a maintainer request for the conflict module to the main maintainer @bdragon, but didn't get any response yet. Three way merge will not be sufficient when dealing with nested inline entity forms, I think, and also we'll need to let the user decide how does she wants to merge when the same field of the same translation has been edited. I think it will be good if we start working together on this? I'll be start working on it approximately in the beginning of the next month.

timmillwood’s picture

We (the workflow initiative) already maintain a 8.x branch of the Conflict module, which was worked on as part of Google Summer of code last year along with the merge library, it definitely needs more work.

hchonov’s picture

I've took a look at it and the Drupal 8 version has only one merge mechanism - it takes 3 revision ids and returns the greatest among them.. So it doesn't need more work, but a complete rewrite from scratch. With whom should I talk about maintainer access rights?

timmillwood’s picture

@hchonov - I think the first step would be to open an issue on the module. Then I will make sure @dixon_ and @jeqq are aware of it.

timmillwood’s picture

No interdiff because we're going in a very different direction by adding an entity_builder method to merge the submitted entity with the default entity revision.

It currently seems to be messing up access (throwing 404 instead of 403) and the moderation form.

timmillwood’s picture

Even though we are merging translations from different revisions, content_moderation_state isn't doing the same, so we're ending up where revisions get different moderation states than they had before.

hchonov’s picture

@timmillwood, the problem is that this way you are preventing the entity changed constraint to fail and also what if the entity has been saved meanwhile by another user with a new default revision for the same translation you are merging? This is where a conflict management is needed so that the user decides how she wants to merge the conflicts.

Status: Needs review » Needs work

The last submitted patch, 26: 2860097-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -122,6 +122,8 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['#entity_builders']['merge_translations'] = '::mergeTranslations';
    

    I suggested an entity builder because I thought this code would live in the CM module. If it's in CEF we can call ::mergeTranslations() directly from CEF::buildEntity().

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
    @@ -14,17 +14,17 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
    +//    if (isset($entity)) {
    

    @hchonov:

    I think this code is only temporarily commented out to avoid the failures caused by #2892132: Entity validation does not always prevent concurrent editing of pending revisions. The plan is postponing this issue on that, if I'm not mistaken. Does this address your concerns?

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -131,6 +133,30 @@ public function form(array $form, FormStateInterface $form_state) {
+          if ($definition->isTranslatable()) {

Mmh, now that I think about it, we need to keep all submitted values, also untranslatable fields, otherwise this will revert any change to those.

hchonov’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -131,6 +133,30 @@ public function form(array $form, FormStateInterface $form_state) {
+      $default_revision = $this->entityTypeManager->getStorage($entity_type)->load($entity->id());
...
+        foreach ($default_revision_translation->getFieldDefinitions() as $field_name => $definition) {
+          if ($definition->isTranslatable()) {
+            $default_revision_translation->set($field_name, $entity->get($field_name)->getValue());
+          }

Consider this code and the following use case.

An entity in the revision 1 has the translatable field title set to "A".

User 1 and 2 both concurrently edit the form. User 1 saves the entity before user 2 with the title field "C". User 2 saves now with title field "B". What happens is that user 2 will override the changes made by user 1 without reviewing them. This should not happen as this was just a simple example but if a user decides to change a field value she might make this decision based on the current value of another field and then such override should not happen. This override also might revert changes on fields made by user 1, which should not happen as well.

I hope this makes sense now :).

timmillwood’s picture

@hchonov - right, we need to update the validator to prevent this from happening, but also allow us to do the translation merges.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
4.69 KB

Mainly a progress patch to see what testbot thinks.

Moved things around as suggested in #30.1, and reversed the merge so we're updating the new entity with translations from default revision, rather than updating default revision with translations from the new entity.

Status: Needs review » Needs work

The last submitted patch, 34: 2860097-33.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
10.5 KB

Adding basic test

Status: Needs review » Needs work

The last submitted patch, 36: 2860097-36.patch, failed testing. View results

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -145,6 +145,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    $this->mergeTranslations($entity);

I think we need to do this only when creating a new revision, otherwise we may override valid translation data.

hchonov’s picture

@timmillwood, how do you want to update the validator to prevent #32 from happening?

And still you are reverting non-translatable fields that have been changed meanwhile, which is not acceptable as well :).

I just don't see how we could make an automatic merge work without notifying the user that something is being merged.

timmillwood’s picture

1. not sure
2. non translatable fields will be updated if the user updates them, they won't be reverted
3. it's still work in progress

hchonov’s picture

Non translatable fields will be reverted in this case :

Both users 1 and 2 concurrently edit an entity form with original value of a non translatable field 'A'. User 1 changes it to 'B' and saves before user 2. Now user 2 saves the form without changing the value leaving it to 'A' in the form values. This leads to user 2 reverting the change made by user 1.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.19 KB
17.88 KB

Fixing some of the failing test and adding test coverage.

Just a progress patch really.

Status: Needs review » Needs work

The last submitted patch, 42: 2860097-42.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

Assigned: Unassigned » plach

Testing/working a bit on this.

timmillwood’s picture

Assigned: plach » Unassigned
Status: Needs work » Needs review
FileSize
11.26 KB
19.56 KB

Still can't get the merging of content_moderation_state entities to work, but thought it was time for a progress patch which includes some changes (https://pastebin.com/P5ZdJqkX) from @plach.

Status: Needs review » Needs work

The last submitted patch, 45: 2860097-45.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Priority: Normal » Major

Hm this sounds at least major, might even be critical as unexpected data loss. Also related to the problem space of #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions, although that issue is specifically about untranslatable fields on the translations.

xjm’s picture

timmillwood’s picture

@xjm - This would be a critical if it wasn't for #2766957: Forward revisions + translation UI can result in forked draft revisions where we mitigate the issue by showing errors / warning preventing any data loss from happening.

In this issue we experimented with possible solutions before settling with the solution in #2766957: Forward revisions + translation UI can result in forked draft revisions. This will now be a where we fix the issue and remove the errors / warnings added in #2766957: Forward revisions + translation UI can result in forked draft revisions.

timmillwood’s picture

xjm’s picture

So the workaround given on the original issue was (from catch):

you can stage multiple translations with this restriction, you just have to publish them all at the same time, and not make changes to the original language while doing so

Does that work?

The summary says:

When adding a new default revision to an entity which has a draft revision on other translations, the draft becomes lost as it has a lower revision id than the default.

The "draft becomes lost" part is the part that sounds like data loss to me. Is that from before #2766957: Forward revisions + translation UI can result in forked draft revisions? What happens now?

Steps to reproduce in the summary would help for reviewing this. Looks like the patch already includes a test (great to see) that documents those steps.

xjm’s picture

Issue tags: +8.4.0 release notes

We should maybe mention this as a known issue for Content Moderation in the 8.4.0 release notes, since (if I understand correctly) this is an 8.4.0-only regression from the hotfix to prevent the data loss from #2766957: Forward revisions + translation UI can result in forked draft revisions.

timmillwood’s picture

Category: Bug report » Task
Issue summary: View changes
Issue tags: -Needs issue summary update

Does that work?

No, the implementation in HEAD that @catch committed in #2766957: Forward revisions + translation UI can result in forked draft revisions allows users to create pending revisions on one translation at a time, once one translation's pending revisions are published pending revisions on another language can be created. Note that this is on a per entity basis, so you can have pending revisions for english on Node/1, and pending revisions for french on node/2, but not pending revisions for english and french on node/1.

The "draft becomes lost" part is the part that sounds like data loss to me. Is that from before #2766957: Forward revisions + translation UI can result in forked draft revisions? What happens now?

Yes, this is from before #2766957: Forward revisions + translation UI can result in forked draft revisions. We were trying to attack the issue from two angles. Currently in HEAD, now that #2766957: Forward revisions + translation UI can result in forked draft revisions is committed there is no data loss, we prevent users from getting into the situation where data loss could occur. Now in this issue we need to convert it from another angle of attack to an actual follow up where we will more or less revert the work done in #2766957: Forward revisions + translation UI can result in forked draft revisions bringing the original issue back, so we can rewrite the revisions+translations system to allow for multiple pending revisions across multiple translations on a single entity.

Switching this to a "task" as the bug is not reproducible anymore.

timmillwood’s picture

@xjm - I'm not sure this is a regression, previously you would lose data, now you get a warning preventing you from losing data. This sounds like an enhancement not a regression. Yes, there is a slight regression in that you can't create multiple pending revisions across translations on an entity anymore, but doing that previously would've meant the pending revisions on all but one of the translations would be lost anyway, so it's not like it was a feature anyone could use.

Gábor Hojtsy’s picture

Issue tags: -8.4.0 release notes
rlnorthcutt’s picture

.

plach’s picture

Assigned: Unassigned » plach

Working on this

Wim Leers’s picture

Title: Only having one default revision per entity forces translations to be kept in sync » [PP-3] Only having one default revision per entity forces translations to be kept in sync
Related issues: +#2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields, +#2920889: EntityChangedConstraintValidator doesn't take adding, removing and reverting translations into account, +#2892132: Entity validation does not always prevent concurrent editing of pending revisions

Updating to reflect #59.

plach’s picture

Title: [PP-3] Only having one default revision per entity forces translations to be kept in sync » [PP-2] Only having one default revision per entity forces translations to be kept in sync
Issue tags: +Needs issue summary update
Related issues: +#2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option.
FileSize
127.67 KB
105.55 KB

Here is a patch ready for testing, it should be functionally complete. It relies on #2892132: Entity validation does not always prevent concurrent editing of pending revisions and #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option., so attached you can find also a the interdiff with those two.

We will probably need to split this into smaller chunks and add some more test coverage. But this is a good time to start looking at it. Tomorrow I will try to open the missing child issues and post patches over there. I'll also update the IS to describe the current work.

plach’s picture

Title: [PP-2] Only having one default revision per entity forces translations to be kept in sync » [PP-3] Only having one default revision per entity forces translations to be kept in sync
Related issues: +#2891215: Add a way to track whether a revision was default when originally created
plach’s picture

plach’s picture

Title: [PP-3] Only having one default revision per entity forces translations to be kept in sync » [PP-4] Only having one default revision per entity forces translations to be kept in sync
Issue tags: -Needs issue summary update +API addition

Updated IS

plach’s picture

Issue summary: View changes

Now for reals

plach’s picture

plach’s picture

Title: [PP-4] Only having one default revision per entity forces translations to be kept in sync » [PP-5] Only having one default revision per entity forces translations to be kept in sync
FileSize
128.8 KB
62.07 KB
1.49 KB

Split off also the API addition: #2924724: Add an API to create a new revision correctly handling multilingual pending revisions. Attached you can find an updated combined patch fixing one issue found while testing this.

plach’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes
Issue tags: -API addition
plach’s picture

plach’s picture

plach’s picture

Title: [PP-5] Only having one default revision per entity forces translations to be kept in sync » [PP-4] Only having one default revision per entity forces translations to be kept in sync
plach’s picture

plach’s picture

Updated patch with the latest changes from the various dependencies.

plach’s picture

Title: [PP-4] Only having one default revision per entity forces translations to be kept in sync » [PP-5] Only having one default revision per entity forces translations to be kept in sync
Issue summary: View changes
plach’s picture

plach’s picture

FileSize
44.95 KB

Sorry, the review patch was wrong, here's the correct one. The combined patch in #77 is ok.

plach’s picture

Title: [PP-5] Only having one default revision per entity forces translations to be kept in sync » [PP-4] Only having one default revision per entity forces translations to be kept in sync
sdewitt’s picture

@plach @timmillwood based on the above comments, I can't tell whether the latest patch/solution will change the current practice/process of revision having rows for every translation of the entity even though the revision/change only affects one of the langcodes (which I think is why revision_translation_affected exists?). Will this patch change that?

I admit to not being able to completely follow the complexity of the discussion above so apologize if this is evident there and I'm not seeing it.

The reason I ask is related to working on a way to resolve the following issue I opened: https://www.drupal.org/project/drupal/issues/2933202 wherein I'd like to be able to query what revisions/translations a particular condition applies to.

plach’s picture

@plach @timmillwood based on the above comments, I can't tell whether the latest patch/solution will change the current practice/process of revision having rows for every translation of the entity even though the revision/change only affects one of the langcodes (which I think is why revision_translation_affected exists?). Will this patch change that?

Nope, that will not change. What is going to change is what data is stored in every translation row: except for the data of the translation being changed, which will be of course stored as-is, all translation records will be a copy of the ones in the default revision.

sdewitt’s picture

Thanks @plach

plach’s picture

Title: [PP-4] Only having one default revision per entity forces translations to be kept in sync » [PP-2] Only having one default revision per entity forces translations to be kept in sync
plach’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

plach’s picture

Status: Postponed » Needs review
FileSize
133.3 KB
62.75 KB
11.76 KB

Here's a reroll after the latest commits. This also provides additional test coverage and reverts some unneeded changes.

Setting to needs review as we really need to start looking at the code now.

timmillwood’s picture

  1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -70,7 +73,7 @@ public function convert($value, $definition, $name, array $defaults) {
    -      $latest_revision_id = $storage->getLatestRevisionId($value);
    +      $latest_revision_id = $this->getLatestRevisionId($storage, $entity_definition, $value);
    

    Think we need to explain why we're using a custom getLatestRevisionId() rather than the one in ContentEntityStorageBase.

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +90,51 @@ public function convert($value, $definition, $name, array $defaults) {
    +   * @return int
    ...
    +    return (string) $latest_revision_id;
    

    Int or string?

  3. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -145,8 +138,19 @@ public function isLatestRevision(ContentEntityInterface $entity) {
    +        $result = !$latest_revision->wasDefaultRevision();
    

    Think we need a test for when the latest revision is not the default revision but was the default revision.

Wim Leers’s picture

(cross-posted with #88)

Here's an initial drive-by review. Mostly nits.

  1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +90,51 @@ public function convert($value, $definition, $name, array $defaults) {
    +    return (string) $latest_revision_id;
    

    Why would we want to cast this to a string? If anything, I'd expect this to be cast to an int?

  2. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -221,4 +221,19 @@ protected function realSave() {
    +   * @return array
    

    s/array/string[]/

  3. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -176,6 +171,12 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
    +    if (!$content_moderation_state->isNew() && $content_moderation_state->content_entity_revision_id->value != $entity_revision_id) {
    

    !==

  4. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -360,6 +351,23 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +   * Checks whether the entity form should get content moderation.
    ...
    +  protected function isModeratedEntityForm(FormInterface $form_object, $operation = NULL) {
    

    The function name is much clearer than the docblock description :)

  5. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -360,6 +351,23 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +   * @param string $operation
    

    string|null

  6. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -360,6 +351,23 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +      in_array($operation ?: $form_object->getOperation(), ['edit', 'default']) &&
    

    Should be a strict in_array().

    Also: what if I have other operations that should be moderated? Why do we even need to only do this for some operations? Could we blacklist instead?

  7. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -145,8 +138,19 @@ public function isLatestRevision(ContentEntityInterface $entity) {
    +      if ($latest_revision_id != $default_revision_id) {
    

    !==

  8. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -165,7 +168,12 @@ public function isLiveRevision(ContentEntityInterface $entity) {
    +    // Load the entity from the storage since the statically cached object may
    +    // be polluted by local changes.
    

    This sounds very ominous. "local changes", what's that?

  9. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -325,7 +317,7 @@ public function testContentTranslationNodeForm() {
    +    // Publish the english pending revision (revision 5).
    

    Nit: s/english/English/

  10. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -338,13 +330,13 @@ public function testContentTranslationNodeForm() {
    -    // Add a english pending revision (revision 6).
    +    // Add an english pending revision (revision 6).
    

    This is fixing the "a", fine, but then we should also s/english/English/

  11. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -87,6 +87,7 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
    +    $revisionable = $entity_type->isRevisionable();
    

    s/$revisionable/$is_revisionable/

  12. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -121,6 +125,16 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
    +        // need to load the latest affected revision for the each language to be
    

    s/for the each/for each/

  13. +++ b/core/modules/node/src/Controller/NodeController.php
    @@ -187,11 +188,17 @@ public function revisionOverview(NodeInterface $node) {
    +        $current_revision = $vid == $default_revision || (!$current_revision_displayed && $revision->wasDefaultRevision());
    

    s/$current_revision/$is_current_revision/

Gábor Hojtsy’s picture

Title: [PP-2] Only having one default revision per entity forces translations to be kept in sync » [PP-1] Only having one default revision per entity forces translations to be kept in sync
plach’s picture

Title: [PP-1] Only having one default revision per entity forces translations to be kept in sync » Only having one default revision per entity forces translations to be kept in sync
FileSize
72 KB
39.5 KB

After some more manual testing, I verified that this no longer blocked on #2892132: Entity validation does not always prevent concurrent editing of pending revisions. I believe this is a positive side-effect of the fix introduced in #2837022: Concurrently editing two translations of a node may result in data loss for non-translatable fields: since now we are comparing timestamps language by language, we no longer erroneously trigger a validation error when a draft revision is older than the default revision. For this reason I'm posting a new patch that no longer includes the draft validation patch.

The attached patch should address most of #88 and #89. It also fixes a trivial bug in the previous version and adds test coverage for that:

9be891980e DEV 2860097: Cleaned-up EntityConverter.
e3cfb12c3b DEV 2860097: Addressed #88.
892b5bea0e DEV 2860097: Additional clean-up.
14becdc60e DEV 2860097: Improved test coverage.
554610e2ca DEV 2860097: Fixed edge case.
9d1f25b0ae (HEAD -> 8.x-entity-ml_drafts-2860097-plach) DEV 2860097: Optimization.

A few replies:

@timmillwood:

Think we need a test for when the latest revision is not the default revision but was the default revision.

I'm not sure what you mean here: if it's the latest revision and it was the default revision when created, it can't not be the default revision now.

@Wim Leers:

1: Theoretically a revision can have a string ID, see #2933570: Clean up PHP doc @param and @return types for entity revision IDs for a detailed discussion on the topic.
8: TBH I don't remember exactly, but I was having issues because some values in the statically cached entity were conflicting with the following logic that was expecting the values as stored to behave correctly. I will try to revert that change to see whether it's captured by the current test coverage.
11/13: This is due to my Java background: there the is prefix would be used only for the accessor method, while the property would be just $revisionable. Not sure how much consistent we are in the user of the "is_" prefix for boolean variables. Do we have a standard for this?

plach’s picture

plach’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 91: entity-ml_drafts-2860097-91.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

I will try to revert that change to see whether it's captured by the current test coverage.

Great!

Do we have a standard for this?

I don't think we do. But note the code I cited did not include the actual (protected/internal) property, it's citing code where you're caching the result of the accessor method. Otherwise I'd agree.


#91 interdiff review:

  1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -71,8 +83,9 @@ public function convert($value, $definition, $name, array $defaults) {
    -    if ($entity instanceof ContentEntityInterface && !empty($definition['load_latest_revision']) && $entity_definition->isRevisionable()) {
    ...
    +    if ($entity instanceof RevisionableInterface && !empty($definition['load_latest_revision']) && $entity_definition->isRevisionable()) {
    

    IIRC I've been told before I should not ever do instanceof RevisionableInterface, because the interface cannot be relied upon. Instead, one should always use \Drupal\Core\Entity\EntityType::isRevisionable().

    (I think it might've been Berdir who said that in code review?)

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -91,34 +104,36 @@ public function convert($value, $definition, $name, array $defaults) {
    +      // is fine to load the latest revision instead. This will allow to handle
    +      // all cases that do not work with pending revisions and always expect the
    

    will allow to handle all cases that do not work with pending revisions sounds pretty scary. That sounds like there are edge cases we're simply not handling. What are those edge cases? And are they indeed not handled here? What happens when one of those cases occurs?

  3. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php
    @@ -205,210 +207,218 @@ public function testTranslateModeratedContent() {
    -    $edit = [
    -      'title[0][value]' => 'Test 1.1 EN',
    -      'langcode[0][value]' => 'en',
    -      'moderation_state[0][state]' => 'published',
    -    ];
    -    $this->drupalPostForm('node/add/article', $edit, t('Save'));
    ...
    +    $node = $this->submitNodeForm('Test 1.1 EN', 'published', 'en');
    

    This is definitely an improvement. But it also is pure refactoring and hence feels out of scope. Can we perhaps land this first, in a separate issue, without functional changes? It'd make this patch simpler & smaller.

  4. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -347,14 +347,16 @@ public function add(LanguageInterface $source, LanguageInterface $target, RouteM
    +    // form initial values may not be up to date.
    

    "form initial values" sounds strange, perhaps "initial form values" or "form's initial values" is better?

  5. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -347,14 +347,16 @@ public function add(LanguageInterface $source, LanguageInterface $target, RouteM
    +      if ($revision_id != $entity->getRevisionId()) {
    

    Can we use !== here?


#91 patch review:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -223,7 +223,7 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
    +    if (!$entity->isNew() && !$entity->isDefaultRevision() && $entity->isTranslatable() && ($entity->getTranslationLanguages(FALSE) || $this->isAnyRevisionTranslated($entity))) {
    

    Just making sure I understand this correctly:

    if ($entity->getTranslationLanguages(FALSE))
     {…}

    is functionally equivalent to a (non-existent but hypothetical) if ($entity->hasAnyTranslation()) { (for a non-default language, because the "inherent" translation must always use the default language), right?

  2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +103,53 @@ public function convert($value, $definition, $name, array $defaults) {
    +   * When dealing with translatable entity types, this attempts to retrieve the
    +   * latest revision ID affecting the translation matching the current content
    +   * language. If none can be found, this falls back to the latest revision ID.
    

    "attempts […] falls back"

    Does this mean that there are edge cases where this "best possible behavior but with some unfixable edge cases" logic actually could cause problems?

    Or is this merely saying "attempts" because there may not yet be any translations, in which case it is 100% safe to fall back to the latest revision ID?

  3. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +103,53 @@ public function convert($value, $definition, $name, array $defaults) {
    +   *   The latest revision ID for the specified entity.
    

    If my previous comment is accurate, this should become:
    The latest translation-affecting revision ID for the specified entity, or just the latest revision ID if the specified entity does not have any translations yet.

  4. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +103,53 @@ public function convert($value, $definition, $name, array $defaults) {
    +      // If the latest translation-affecting revision was a default revision, it
    +      // is fine to load the latest revision instead. This will allow to handle
    

    Why is this fine? Can you @see some other documentation perhaps?

  5. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +103,53 @@ public function convert($value, $definition, $name, array $defaults) {
    +      // all cases that do not work with pending revisions and always expect the
    +      // latest revision being loaded (and being the default revision).
    

    "This [fallback behavior] will allow to handle all cases that do not work with pending revisions […]"

    This makes it sound like we only perform this fallback behavior to automagically fix existing code with broken assumptions/behavior/logic?

  6. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -87,6 +103,53 @@ public function convert($value, $definition, $name, array $defaults) {
    +    // Fall back to the latest revisions if no affected revision for the current
    +    // language could be found. This is acceptable as it means the entity is not
    

    "current language" — oh wait, all this logic essentially varies by the languages:language_content cache context!!

    The docblock *and* the interface both suggest this works regardless of whatever the currently active content language is!

    At minimum, the docblock should be clarified. Ideally, getLatestRevisionId() should receive a langcode to prevent any disambiguity. I see that this is at least a protected method, so insisting on making the interface clearer probably is not as impactful, but I do think it'd still be better for long-term maintainability.

  7. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -118,6 +118,15 @@ function content_moderation_entity_translation_delete(EntityInterface $translati
    +function content_moderation_entity_prepare_form(EntityInterface $entity, $operation, FormStateInterface $form_state) {
    +  \Drupal::service('class_resolver')
    +    ->getInstanceFromDefinition(EntityTypeInfo::class)
    +    ->entityPrepareForm($entity, $operation, $form_state);
    +}
    

    I have no idea what this does and why.

  8. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -251,10 +261,16 @@ function content_moderation_entity_bundle_info_alter(&$bundles) {
    +          // If we have even one translatable bundle for moderated entity types,
    

    s/moderated entity types/a moderated entity type/

  9. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -176,6 +171,12 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
    +    if (!$content_moderation_state->isNew() && $content_moderation_state->content_entity_revision_id->value !== $entity_revision_id) {
    
    1. Why does this not use isNewRevision()?
    2. Why was this code moved away from its previous place?
  10. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -245,23 +246,19 @@ public function entityTranslationDelete(EntityInterface $translation) {
    -    if (!$this->moderationInfo->isLatestRevision($entity)) {
    -      return;
    -    }
    

    Why is this being removed? Because it now is guaranteed to be the latest revision?

  11. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -245,23 +246,19 @@ public function entityTranslationDelete(EntityInterface $translation) {
    +    // However it should be displayed if the latest affected revision is a
    +    // pending revision.
    +    if (!$entity->isDefaultRevision() && (($entity->wasDefaultRevision() && $this->moderationInfo->isDefaultRevisionPublished($entity)) || !$entity->isLatestTranslationAffectedRevision())) {
    

    The comment does not match the if-test

  12. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -245,23 +246,19 @@ public function entityTranslationDelete(EntityInterface $translation) {
    -
         $component = $display->getComponent('content_moderation_control');
    

    Supernit: unnecessary whitespace change.

  13. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -268,6 +270,37 @@ public function entityBaseFieldInfo(EntityTypeInterface $entity_type) {
    +  public function entityPrepareForm(EntityInterface $entity, $operation, FormStateInterface $form_state) {
    

    This feels out of place in this class. Why not have this live directly in the hook_form_alter() implementation that calls this?

  14. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -360,6 +351,23 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +      in_array($operation ?: $form_object->getOperation(), ['edit', 'default'], TRUE) &&
    

    See #89.6.

  15. +++ b/core/modules/content_moderation/src/Form/EntityModerationForm.php
    @@ -138,6 +138,9 @@ public function buildForm(array $form, FormStateInterface $form_state, ContentEn
    +    /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
    +    $storage = \Drupal::entityTypeManager()->getStorage($entity->getEntityTypeId());
    +    $entity = $storage->createRevision($entity, $entity->isDefaultRevision());
    

    So this is Content Moderation force-auto-creating-a-new-revision, right?

    This feels like the wrong place to do this, because it'll only work for content created/updated via the UI/via forms. It won't work for content created/updated via HTTP APIs. (IOW: this negatively impacts the API-First Initiative AFAICT.)

  16. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -154,8 +158,7 @@ public function hasPendingRevision(ContentEntityInterface $entity) {
       public function isLiveRevision(ContentEntityInterface $entity) {
         $workflow = $this->getWorkflowForEntity($entity);
    -    return $this->isLatestRevision($entity)
    -      && $entity->isDefaultRevision()
    +    return $entity->isDefaultRevision()
    

    Doesn't this change mean that the interface docs for this method are no longer accurate?

  17. +++ b/core/modules/content_translation/src/Routing/ContentTranslationRouteSubscriber.php
    @@ -70,6 +70,7 @@ protected function alterRoutes(RouteCollection $collection) {
    +              'load_latest_revision' => TRUE,
    

    I'd imagine that loading the latest revision is the default behavior. So … are these changes even necessary?

    And if they are, isn't that a BC break?

  18. +++ b/core/modules/node/src/Controller/NodeController.php
    @@ -173,6 +173,7 @@ public function revisionOverview(NodeInterface $node) {
    +    $current_revision_displayed = FALSE;
    
    @@ -187,11 +188,17 @@ public function revisionOverview(NodeInterface $node) {
    +          $current_revision_displayed = TRUE;
    

    This boolean isn't used anywhere.

  19. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
    @@ -224,6 +224,7 @@ function entity_test_entity_bundle_info_alter(&$bundles) {
       }
    +
     }
    

    Supernit: unnecessary whitespace change.

plach’s picture

Wow, another big one ;)

A few more replies (tldr; I have a couple of issues still to be addressed, but I want to trigger separate test runs):


(interdiff review)
1: RevisionableInterface cannot be relied upon when you need to know whether the entity type schema is revisionable, which needs indeed ::isRevisionable(). However it is a good way to check whether the methods we need to invoke subsequently are available. The entity type check is performed as third test in that if.
2: See below.
3: Those test lines were added in the patch, so those changes are actually making the patch smaller, if not simpler ;)
4: Fixed, although I suspect that that change is not actually required now that all CT routes use the latest revision flag. I will try to remove that.
5: Nope, revision ID types are tricky (see #2933570: Clean up PHP doc @param and @return types for entity revision IDs): they are inconsistently passed/returned as int or string depending on code paths.


(patch review)
1. Yep, exactly. We want to trigger the revision translation merge logic even when a new translation has not been stored yet.
2. The latter: it's perfectly fine to have a NULL value returned there because, as you say, the requested translation may not exist. In that case the next best return value is the latest revision ID. We always want to create translations starting from source values as they appear in the latest revision. At least this is the most sensible behavior wrt how CM works.
3: Fixed. Clarified the difference between the two supported cases (non translatable vs not actually translated).
4/5/6: I'll try to clarify that comment, but I need to think about it and want to trigger a new test run first.
7: It's a way to instantiate a non-service class implementing ContainerInjectionInterface without messing with the container directly. This is a pattern used widely throughout CM's codebase, so I thought it would make sense to use it for consistency.
8. I tried to clarify the comment
9.1: Because the previous code wasn't :) Probably because that's the only place responsible to set that flag, so it would never evaluate to TRUE.
9.2: Because the new recommended way to create a new revision is actually instantiating a new revision object via the ::createRevision() method, but that needs to be applied to the correct translation object, which is instantiated in the lines immediately before.
10: Because the moderation form should be displayed also on revisions not being the latest one, when translated entities are involved. You can have only one latest revision, but multiple "active" drafts in various languages.
11: I tried to clarify the comment
12: Fixed
13: The prepare form phase is the phase dedicated to prepare entity values before they are populated into form elements. This makes it sure that any form alteration always receives the correct entity object. Aside from that, I believe that logic belongs to this class, since also the ::formAlter() logic si defined there.
14: This code has been factored out from the existing logic in the ::formAlter() method, that was already hardcoding the default and edit operations. Anyway, given that entity forms may cover vastly different operations, I believe it would be risky to assume that any operation but edit/default (e.g. delete) would be even remotely compatible with the logic we are introducing there. Hence IMO a whitelist is the way to go.
15: Could you suggest a better place? According to the discussion taking place in #2937850: Mark revision_default as internal for REST consumers, I believe we will need an explicit endpoint to trigger the creation of a new revision via REST. I proposed the introduction of the ::createRevision() exactly to help programmatic workflows reusing the same logic we have in the UI. However, since it implies the instantiation of a new object, which is a best practice to avoid changes being unintentionally applied to the wrong revision object, we cannot simply set the new_revision flag under the hood via a random hook, e.g. hook_entity_presave(). I'm not sure this would even be a good idea honestly, since the creation of a new revision is a non-trivial operation and one people should be explicitly performing. Aside from that, the moderation form, is just a UI shortcut to change only the moderation state, so it translates to a simple POST request, if dealing with a new revision object.
16: Good point, I misunderstood the meaning of "live" ;) I will try to update the code to still work but to be compatible with the interface definition.
17: Nope, so far CTs has been happily ignoring pending revisions and has been assuming to be always dealing with default revisions. I don't think this change qualifies as a BC break, just a behavior change due to a bug fix. It's enforcing a correct behavior, basically, since only the latest (translation-affecting) revision is supposed to be edited, at least until we support multiple parallel pending revisions.
18: I guess you missed this :)

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -187,11 +188,17 @@ public function revisionOverview(NodeInterface $node) {
+        $current_revision = $vid == $default_revision || (!$current_revision_displayed && $revision->wasDefaultRevision());

19: Fixed


Regarding #89.8, test coverage indeed covers that, see https://www.drupal.org/pift-ci-job/866588. I will try to dig deeper and improve the related comment.


This should address the most of #95 and fix previous test failures:

8cecb305a8 DEV 2860097: Fixed entity converter test failures.
22f55406f3 DEV 2860097: Fixed CM test failures.
335b3f66e9 (HEAD -> 8.x-entity-ml_drafts-2860097-plach) DEV 2860097: Addressed #95.

Edit: fixed a few minor grammatical issues and typos

plach’s picture

Status: Needs work » Needs review
Wim Leers’s picture

(interdiff review)

  1. Looking forward to that!

(patch review)

  1. 👍 Thanks for confirming!
  2. Thanks, this clarification makes sense. But I think your explanation here is clearer than in the one in the patch. Could you use the best bits of both?
  3. 👍
  4. Looking forward to that!
  5. Looking forward to that!
  6. Looking forward to that!
  7. 👍 — I see now that this is a pre-existing pattern. (also point 13)
  8. Better, but I think it can be simplified: s/being translatable/translatable/
  9. 👍
  10. 👍
  11. Hm, I understand most of the sentence, but the part after the comma makes me wonder if that's correct. The part before the comma says that the latest revision in each translation should show the moderation form. This makes sense. But then the bit after the comma says unless they were created as default. What if the latest revision that affected some translation X is also the default revision, why shouldn't that have the moderation form?
  12. 👍
  13. 👍 — I see now that this is a pre-existing pattern. (also point 7)
  14. 👍 — confirmed to be a pre-existing problem, and there even is an issue for it already: #2935939: Content Moderation hardcodes the 'edit' and 'default' form modes — other form modes are not supported. I've transplanted your comment to that issue: #2935939-4: Content Moderation hardcodes the 'edit' and 'default' form modes — other form modes are not supported.
  15. I don't know Entity/Field API well enough to suggest what the right place would be. But upon re-reading previous issues where Content Moderation affected API-First (#2843753: Prevent ContentModerationState from being exposed by REST's EntityResource and #2779931: Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access), it's become clear that basically Content Moderation has zero exposure so far in API-First responses. This does not change that. So we don't need to solve this here, nor should we or even can we. Particularly, in #2779931: Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access it was made clear that there is no reason to ever interact with ContentModerationState entities directly. Fine. But now there is … no way it is exposed in HTTP APIs at all, and no clear way to do so. Of course, that's a pre-existing problem. So, ignore this remark of mine, it's simply out of scope. I'm sorry that I brought this up and got you on a tangent!
  16. Looking forward to that!
  17. AFAICT this can then be done in a separate issue? We can land that separately easily. We probably also want test coverage for it?
  18. 😅 👍
  19. 👍

Regarding #89.8, test coverage indeed covers that, see https://www.drupal.org/pift-ci-job/866588. I will try to dig deeper and improve the related comment.

Looking forward to that!

Wim Leers’s picture

RE #96.17/#98.17: it looks like this is actually a follow-up for #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option.?
RE #95.3: you're right, it's not possible to move the test changes to a separate issue.

Another review round, now with a slightly better understanding. (But I still think that an actual Entity/Field API maintainer should also review this. And preferably Workflow Initiative members @timmillwood and @amateescu.)

  1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -46,14 +51,24 @@ class EntityConverter implements ParamConverterInterface {
    -  public function __construct(EntityManagerInterface $entity_manager) {
    +  public function __construct(EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager) {
    

    Given that we apparently have so many subclasses of this even just in core, I think it's fair to say that contrib will also have these. Which means we should be careful to not break BC and make this an optional parameter.

  2. +++ b/core/modules/content_moderation/src/Entity/Routing/EntityModerationRouteProvider.php
    @@ -88,7 +88,7 @@ protected function getLatestVersionRoute(EntityTypeInterface $entity_type) {
    -            'load_pending_revision' => 1,
    +            'load_latest_revision' => 1,
    
    +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
    @@ -83,23 +81,8 @@ protected function isEditFormPage(Route $route) {
    +    // @todo Deprecate this in https://www.drupal.org/node/2924812.
    

    We wouldn't need these anymore if #2924812: Deprecate EntityRevisionConverter in content_moderation. had already landed.

    Also, let's use TRUE instead of 1, otherwise this will A) be inconsistent, B) conflict with #2924812.

    I posted a reviewed of that issue (RTBC!).

  3. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -268,6 +270,37 @@ public function entityBaseFieldInfo(EntityTypeInterface $entity_type) {
    +      // Restore the revision ID as other modules may expect to find it still
    +      // populated. The entity object wil be marked as a new revision on
    +      // presave.
    

    This would benefit from an explicit @see, because it's not clear to me where exactly this is happening. My best guess is that this is referring to \Drupal\content_moderation\Entity\Handler\ModerationHandler::onPresave()?

  4. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -360,6 +351,23 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +  protected function isModeratedEntityEditForm(FormInterface $form_object, $operation = NULL) {
    ...
    +      in_array($operation ?: $form_object->getOperation(), ['edit', 'default'], TRUE) &&
    

    This $operation parameter is optional, and if it's NULL, then $form_object is supposed to hold the operation. But nothing guarantees this.

    Can we refactor this small bit of complexity away?

  5. +++ b/core/modules/node/src/Controller/NodeController.php
    @@ -187,11 +188,17 @@ public function revisionOverview(NodeInterface $node) {
    +        // We treat also the latest affected revision as current revision, if it
    

    s/affected/translation-affected/

effulgentsia’s picture

Would it be possible to split out everything that's in this patch that's outside of content_moderation module into a separate issue? I'd like to make sure that that gets focused thorough review first, since it affects non-experimental code. Once that's committed, I think the content_moderation changes will be easier to review, since that's still an experimental module, so we don't need to be quite as careful with BC as we do with non-experimental code.

xjm’s picture

#101 sounds like a good idea. I agree we have a little more leeway for Content Moderation itself. Normally the beta is the hard deadline for additions, but there's a bit of a gray area between the minor's beta and RC for experimental module bugfixes when the modules are beta but almost RC themselves. Plus, on a quick scan of the patch, the CM changes seem to be mostly internal with just one API addition that I saw.

plach’s picture

#95.4, interdiff review:

4: Fixed, although I suspect that that change is not actually required now that all CT routes use the latest revision flag. I will try to remove that.

That change is actually required to manage one edge case. I added test coverage for that (ModerationLocaleTest::testNewTranslationSourceValues()).


#98:

2: Could you suggest a merged version? I'd like to avoid explicitly mentioning entity forms and Content Moderation, since those are just a single use case, although an important one.
8: Fixed
11: Moderation forms are not displayed on default revisions when they are published, because that is considered a "terminal" state for the workflow. In practice there can be other state transitions, but normally that's where a workflow cycle ends.
16: I have reworked the logic governing the moderation form display and restored the original logic for "live" revisions. Added a lot of test coverage for this, as I realized not everything was covered by existing multilingual moderation tests.
17: Yep
19: Instead of reworking the comment, I reworked the fix: basically the error was triggered when adding a new translation to the default revision, since in that case there would be no moderation state yet, thus making the following lines to fail.


#100:

1: Fixed
2: Fixed
3: Fixed
4: Well, the entity form object $operation will be always populated by the entity type manager when instantiating the form object, that should a safe fallback value. Additionally, since that logic was already there, I'd prefer not to change it.
5. Fixed


To address #98.17, #100 and #101, I will now split this patch in a few chunks:

  • A new blocker taking care of the entity converter changes + the related Content Translation changes.
  • A follow-up issue taking care of the node revision UI changes: IIRC those are making the UI work better, but they are not strictly required.

However, I'd like to keep some of the changes to Content Translation here, because the related test coverage is provided by the additional Content Moderation test added here.

plach’s picture

Title: Only having one default revision per entity forces translations to be kept in sync » [PP-1] Only having one default revision per entity forces translations to be kept in sync
Issue summary: View changes
Related issues: +#2938895: Make EntityConverter load the latest translation-affecting revision for translated entities
Wim Leers’s picture

#98

  1. Suggestion:
    - For an entity of an untranslatable entity type, this just returns ID of the latest revision.
    - For an entity of a translatable entity type that does not have any translations yet, this also just returns the ID of the latest revision.
    - For an entity of a translatable entity type that does have a translation in the specified langcode, this determines the latest revision that did affect the translation with that langcode.
    

    … but actually, the docs for @return state pretty much the same thing. I think we can perhaps just remove the paragraph I questioned, rather than replacing it with my suggestion… :) What do you think?

  1. 👍
  1. You restored the original logic, so the previous change was wrong? If so, why did it pass tests? This is probably why you added test coverage. Can we move the adding of those tests to a separate issue that we can get committed faster?
  2. "Yep" what? "Yep this can be done in a separate issue" or "yep we want test coverage for it"? Based on later in the comment: "yep, we want a separate issue for it"
  1. 👍 (This addressed #89.8.)

#100:

  1. Great, but … you haven't reverted the changes in the EntityConverter subclasses yet. Leaving those changes for a follow-up would make this patch quite a bit smaller.
  2. 👍
  3. 👍
  4. That's not true, the original logic was:
    elseif ($form_object instanceof ContentEntityFormInterface && in_array($form_object->getOperation(), ['edit', 'default'])) {
    

    i.e. that always relied on $form_object->getOperation(), rather than receiving an explicit $operation parameter.

  5. 👍

  1. Interdiff review yielded only one stupid nit, this one:
    +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -64,11 +62,18 @@ class EntityConverter implements ParamConverterInterface {
        * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    

    Nit: needs |null.

  2. Working on #98.2 made me realize something though: this patch makes the load_latest_revision flag no longer triggers the loading of just the latest revision. Because the latest revision is a well-defined concept: \Drupal\Core\Entity\RevisionableStorageInterface::getLatestRevisionId() + \Drupal\Core\Entity\Query\QueryInterface::latestRevision() assign very specific meaning to this … and entity.api.php made it even more specific:
     * The "latest revision" of an entity is the most recently created one,
     * regardless of it being default or pending. If the entity is translatable,
     * revision translations are not taken into account either. In other words, any
     * time a new revision is created, that becomes the latest revision for the
     * entity overall, regardless of the affected translations. To load the latest
    […]
    

    I think it'd be fair to say that developers would expect that load_latest_revision would match all these docs. But as of this patch, it won't. In fact, it now behaves as if it were named load_latest_translation_affected_revision! Which is also something that is a well-defined concept: \Drupal\Core\Entity\TranslatableRevisionableStorageInterface::getLatestTranslationAffectedRevisionId(), and entity.api.php made that even more clear:

     * The "latest translation-affected revision" is the most recently created one
     * that affects the specified translation. For example, when a new revision
     * introducing some changes to an English translation is saved, that becomes the
     * new "latest revision". However, if an existing Italian translation was not
     * affected by those changes, then the "latest translation-affected revision"
     * for Italian remains what it was. To load the Italian translation at its
     * latest translation-affected revision:
    […]
    

    The fact that #103 renamed EntityConverter::getLatestRevisionId() to EntityConverter::getLatestTranslationAffectedRevision() only seems to confirm this?

    (Note that you said We always want to create translations starting from source values as they appear in the latest revision. At least this is the most sensible behavior wrt how CM works. — which now made me realize that we're making something generic behave in a way that fits Content Moderation, but that probably does not make sense in the grand scheme of things.)

plach’s picture

Title: [PP-1] Only having one default revision per entity forces translations to be kept in sync » [PP-1] Ensure that content translations can be moderated independently

Here is the follow-up to handle the node revision UI: #2938947: Always list a "Current revision" for each available content translation.

Wim Leers’s picture

Wim Leers’s picture

plach’s picture

@Wim Leers, #105.#98:

16: Tests were passing because they were not testing the new stuff: it was not possible to create multiple pending revisions per language before. Hence we cannot split the new test coverage to a dedicated issue, it’s coupled with the CM changes.


#105.#100:

1: As we discussed, I moved everything to the blocker issue.
4: Ah, right, sorry, I thought I copied everything as-is. Fixed.


I had a call with @Wim Leers to discuss his concern and I clarified that EntityConverter behavior is actually a mix of the latest revision and latest translation-affecting revision behavior, because it always returns a revision, whereas the latter can return NULL if no matching revision translation is found. Given that for non translated entities, we fall back to loading the latest revision, we agreed that it's ok to keep the current state, also because otherwise we would need an explicit flag for multilingual use cases, which would be likely to be left behind. We agreed that improving the documentation of EntityConverter should be enough.


Attached you can find a review patch and a combined patch. All the node-related changes will be posted at #2938947: Always list a "Current revision" for each available content translation.

Wim Leers’s picture

So, I don't feel I can RTBC this, but I think I helped get the patch in a better shape. I won't catch certain classes of problems in the patch though, because I'm not an Entity/Field API expert nor a Content Moderation expert. I think it'd be great if at least the Content Moderation maintainer (Tim Millwood) could review this. So I pinged him on IRC.


But here's one more scan of the patch:

  1. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php
    @@ -191,4 +202,358 @@ public function testTranslateModeratedContent() {
    +    $this->assertNotEquals($node->toUrl('latest-version', ['absolute' => TRUE])->toString(), $this->getSession()->getCurrentUrl());
    

    Supernit/FYI: you could use setAbsolute()

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -60,6 +60,13 @@ function content_translation_module_implements_alter(&$implementations, $hook) {
    +    // Move our hook_entity_bundle_info_alter() implementation to the top of the
    +    // list.
    

    Can we document why?

  3. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -330,8 +344,21 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
    +    // initial form values may not be up to date.
    

    Nit: s/up to date/up-to-date/

hchonov’s picture

I still have to catch up a lot, but there is something that is not really clear to me. If someone is currently using forward revisions for whatever use case and is relying on the fact that Drupal will always load the entities in their default revisions at all the places, then after this patch this behavior will suddenly change and e.g. on content translation routes unexpectedly the forward revisions will be loaded instead of the default ones. I am sorry if I don't understand this correctly, but if I do, then I don't think that this is right.

If there is a need for such a feature - loading the newest instead the default revision, then it should be an optional behavior and not the default one.

I see that @Wim Leers has asked about the change on the content translation routes in #95.17:

I'd imagine that loading the latest revision is the default behavior. So … are these changes even necessary?

And if they are, isn't that a BC break?

and @plach has responded in #96.17:

Nope, so far CTs has been happily ignoring pending revisions and has been assuming to be always dealing with default revisions. I don't think this change qualifies as a BC break, just a behavior change due to a bug fix. It's enforcing a correct behavior, basically, since only the latest (translation-affecting) revision is supposed to be edited, at least until we support multiple parallel pending revisions.

Unfortunately, I have to disagree and I consider it a BC break. This is no BC break from the content moderation point of view only and content moderation is not necessary the only use case for forward revisions.

effulgentsia’s picture

Title: [PP-1] Ensure that content translations can be moderated independently » Ensure that content translations can be moderated independently
FileSize
57.85 KB

I committed #2938895: Make EntityConverter load the latest translation-affecting revision for translated entities, so reuploading #109's ".review" patch as the new main patch.

effulgentsia’s picture

Drupal will always load the entities in their default revisions at all the places then after this patch this behavior will suddenly change

Not all the places. Just where the load_latest_revision route flag is set to true.

e.g. on content translation routes unexpectedly the forward revisions will be loaded instead of the default ones.

For content translation, that flag did just now get added (and committed) in #2938895: Make EntityConverter load the latest translation-affecting revision for translated entities. Please comment there or open a followup if you want that line reverted or discussed further.

Wim Leers’s picture

@hchonov: thanks for the review! I'm glad you're reviewing, you know far more about this than I do :) Just one question for you: is #111 the only concern you have, or is that just the one that really caught your eye? In other words: are you fine with everything else in the patch?

plach’s picture

@hchonov:

If there is a need for such a feature - loading the newest instead the default revision, then it should be an optional behavior and not the default one.

That's a good point. Honestly I never thought about pending revisions when writing the CT code, and I should have because they were supported by core way before 8.0.0 was released. Hence I see this issue as a feature completion (or a bug fix) rather than a behavior change. However, if for some reason the contrib/custom space were relying on this incomplete behavior, it could indeed experience some malfunctioning now. Given that loading the latest (translation-affecting) revision is needed only when Content Moderation is enabled, at least in core, we could alter CT's route definitions and add the load_latest_revision flag in the CM's code. What do you think?

timmillwood’s picture

Generally looks awesome, just a couple of small nitpicks:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -223,7 +223,7 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
    +    if (!$entity->isNew() && !$entity->isDefaultRevision() && $entity->isTranslatable() && ($entity->getTranslationLanguages(FALSE) || $this->isAnyRevisionTranslated($entity))) {
    

    I feel this needs a bit more documentation. Have these changes changed the meaning of the comment above?

  2. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -245,27 +246,28 @@ public function entityTranslationDelete(EntityInterface $translation) {
    -    // Don't display the moderation form when when:
    -    // - The revision is not translation affected.
    -    // - There are more than one translation languages.
    -    // - The entity has pending revisions.
    

    It's a shame to lose this documentation.

plach’s picture

This addresses #110 and #116 and reverts another unneeded change.

@timmillwood:

#116.2: Actually the logic there changed slightly, so that comment was changed into the following, as it was no longer accurate:

    // The moderation form should be displayed only when viewing the latest
    // (translation-affecting) revision, unless it was created as published
    // default revision.
timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

It's be nice to get more reviews (@sam152) and more agreement (@hchonov), but I think we're RTBC ready, even commit ready as any other issues might be considered minor and easier to fix in follow ups rather than re-rolling a 58Kb patch each time.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
58.2 KB
656 bytes

Another minor change.

@timmillwood:

Thanks for the RTBC, but I'd prefer to give @hchonov the possibility to fully review the patch and think about #115. I had a long discussion about that with Wim in Slack, I'm working on a summary.

amateescu’s picture

  1. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -268,6 +270,38 @@ public function entityBaseFieldInfo(EntityTypeInterface $entity_type) {
    +      $storage = $this->entityTypeManager->getStorage($entity->getEntityTypeId());
    

    This doesn't seem to be used more than once, why do we explicitly assign a variable for it?

    Also, the code in this method is quite compact and hard to read, can we add some empty lines in there?

  2. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -360,6 +352,21 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +      in_array($form_object->getOperation(), ['edit', 'default'], TRUE) &&
    

    Why do we hardcode these two operations here?

    The !$entity->isNew() check from entityPrepareForm() above should ensure that we don't try to create new revisions in "entity create" forms..

    Oh, I see this was already discussed above and punted to #2935939: Content Moderation hardcodes the 'edit' and 'default' form modes — other form modes are not supported.

  3. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
    @@ -176,8 +176,8 @@ protected function opSimple() {
    -        // join the data table.
    -        $entity_base_table = $entity_type->isTranslatable() ? $entity_type->getDataTable() : $entity_type->getBaseTable();
    +        // join the base table.
    +        $entity_base_table = $entity_type->getBaseTable();
    
    @@ -187,12 +187,6 @@ protected function opSimple() {
    -          if ($entity_type->isTranslatable()) {
    -            $configuration['extra'][] = [
    -              'field' => $entity_type->getKey('langcode'),
    -              'left_field' => $entity_type->getKey('langcode'),
    -            ];
    -          }
    
    +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
    @@ -121,10 +121,10 @@ public function testStateFilterViewsRelationship() {
    -    // Four revisions for the nodes when no filter.
    -    $this->assertNodesWithFilters([$node, $second_node, $third_node, $third_node], []);
    +    // The three default revisions are listed when no filter is specified.
    +    $this->assertNodesWithFilters([$node, $second_node, $third_node], []);
    

    Are these two changes related?

    I guess the first hunk makes the view not show translations anymore, right? But is that behavior change correct?

As an aside, does this patch happen to fix #2927455: When adding a new translation the entity should start from the draft state as well?

hchonov’s picture

Re #114:
@Wim Leers:
That was the first, that caught my attention :). Review of the current patch at the end of my comment :).

Re #115:
@plach:

That's a good point. Honestly I never thought about pending revisions when writing the CT code, and I should have because they were supported by core way before 8.0.0 was released.

CT and core cannot cover all possible use cases of forward revisions. What happened here was a declaration of the content moderation case as the default behavior of forward revisions, but we should not enforce this. Instead we should make it possible for other modules to hook in and if needed e.g. exchange the default revision with a forward revision. It would be really nice if this is considered each time changes have to be made that are regarding forward revisions.
As a simple example - we could just think about what would happen if autosave is creating forward revisions.

Given that loading the latest (translation-affecting) revision is needed only when Content Moderation is enabled, at least in core, we could alter CT's route definitions and add the load_latest_revision flag in the CM's code. What do you think?

If this is the only way, then I am not going to argue. There is only drawback with this - this makes it impossible for other modules making use of forward revisions to work together with content moderation, but I guess this will be not the only point? If so, then we are fine to go, but probably should somehow make it clear that content moderation could not be used together with other modules/approaches relying on forward revisions.

Re #119:

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -121,6 +125,16 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
+        if ($is_revisionable) {
+          $latest_revision_id = $storage->getLatestTranslationAffectedRevisionId($entity->id(), $langcode);
+          $entity = $latest_revision_id ? $storage->loadRevision($latest_revision_id) : $default_revision;
+          $translations = $entity->getTranslationLanguages();
+        }

@@ -330,8 +344,21 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
+    if (!$entity->isDefaultRevision()) {
+      /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
+      $storage = $this->entityTypeManager()->getStorage($entity->getEntityTypeId());
+      $revision_id = $storage->getLatestTranslationAffectedRevisionId($entity->id(), $source->getId());
+      if ($revision_id != $entity->getRevisionId()) {
+        $entity = $storage->loadRevision($revision_id);
+      }
+    }

Unfortunately this as well could not be the default behavior here, as it is only valid for the content moderation use case, but not if e.g. dealing with autosave forward revisions.
We should make it possible, that other modules hook in and exchange/prepare the entity if needed, but not hard code the logic here.

I am not reviewing the content moderation part, as I am not really familiar with everything there.

Sam152’s picture

@Wim pinged me in IRC and asked me to review. For the parts of this I understand, this looks like amazing work. Here is all I picked up:

  • +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -176,6 +171,12 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
    +    if (!$content_moderation_state->isNew() && $content_moderation_state->content_entity_revision_id->value != $entity_revision_id) {
    +      $content_moderation_state = $storage->createRevision($content_moderation_state, $entity->isDefaultRevision());
    +    }
    
    +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsDataIntegrationTest.php
    @@ -108,14 +108,12 @@ public function testContentModerationStateBaseJoin() {
         $expected_result = [
           [
             'nid' => $node->id(),
    -        // @todo I would have expected that the content_moderation_state default
    -        //   revision is the same one as in the node, but it isn't.
             // Joins from the base table to the default revision of the
             // content_moderation.
    -        'moderation_state' => 'draft',
    +        'moderation_state' => 'published',
             // Joins from the revision table to the default revision of the
             // content_moderation.
    -        'moderation_state_1' => 'draft',
    +        'moderation_state_1' => 'published',
             // Joins from the revision table to the revision of the
             // content_moderation.
             'moderation_state_2' => 'published',
    

    These changes will resolve #2933099: "Default Revision cannot be deleted." But it's not the default revision... at the same time which is nice. I wonder if it's worth rolling the tests from there into here as well. I do wonder if this is a behavioural change in the views integration, but as per #2894479: Deprecate the Views relationship from moderated content to the "content_moderation_state" entity using the join to content_moderation_state has always seemed kind of broken, so maybe this is restoring some order. In any case, there should be no reason to ever use this relationship anymore.

    On another (super painful) note, to 100% resolve the above issue for existing sites, we'd need an upgrade path to shuffle around which is the default content_moderation_state revision :(

  • With all the massive improvements to the underlying API, it'd be great to get #2915398: The moderation_state field is not computed during the creation of a new entity translation. in as well so a relatively superficial bug isn't blocking translations from working properly for workflows that aren't closely modelled off "Editorial".
Wim Leers’s picture

Most remarks in #120+#121+#122 can be addressed. But there's one that's significantly harder:

@Wim Leers in #95.17
I'd imagine that loading the latest revision is the default behavior. So … are these changes even necessary?

And if they are, isn't that a BC break?

@plach in #96.17
Nope, so far CTs has been happily ignoring pending revisions and has been assuming to be always dealing with default revisions. I don't think this change qualifies as a BC break, just a behavior change due to a bug fix. It's enforcing a correct behavior, basically, since only the latest (translation-affecting) revision is supposed to be edited, at least until we support multiple parallel pending revisions.
@hchonov in #111
If someone is currently using forward revisions for whatever use case and is relying on the fact that Drupal will always load the entities in their default revisions at all the places, then after this patch this behavior will suddenly change and e.g. on content translation routes unexpectedly the forward revisions will be loaded instead of the default ones. I am sorry if I don't understand this correctly, but if I do, then I don't think that this is right.

If there is a need for such a feature - loading the newest instead the default revision, then it should be an optional behavior and not the default one.

(and, responding to @plach in #96.17)

Unfortunately, I have to disagree and I consider it a BC break. This is no BC break from the content moderation point of view only and content moderation is not necessary the only use case for forward revisions.

@plach in #115
That's a good point. Honestly I never thought about pending revisions when writing the CT code, and I should have because they were supported by core way before 8.0.0 was released. Hence I see this issue as a feature completion (or a bug fix) rather than a behavior change. However, if for some reason the contrib/custom space were relying on this incomplete behavior, it could indeed experience some malfunctioning now. Given that loading the latest (translation-affecting) revision is needed only when Content Moderation is enabled, at least in core, we could alter CT's route definitions and add the load_latest_revision flag in the CM's code. What do you think?
@hchonov in #121
CT and core cannot cover all possible use cases of forward revisions. What happened here was a declaration of the content moderation case as the default behavior of forward revisions, but we should not enforce this. Instead we should make it possible for other modules to hook in and if needed e.g. exchange the default revision with a forward revision. It would be really nice if this is considered each time changes have to be made that are regarding forward revisions.
As a simple example - we could just think about what would happen if autosave is creating forward revisions.

If this is the only way, then I am not going to argue. There is only drawback with this - this makes it impossible for other modules making use of forward revisions to work together with content moderation, but I guess this will be not the only point? If so, then we are fine to go, but probably should somehow make it clear that content moderation could not be used together with other modules/approaches relying on forward revisions.

My understanding of the discussion & situation:

  1. @hchonov is arguing that adding load_latest_revision to routes is a BC break because it causes behavior to change, which would break modules making different use of forward revisions.
  2. Important to know: #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option. (committed in November) updated \Drupal\Core\Entity\EntityResolverManager::setRouteOptions() to automatically set the load_latest_revision flag on all _entity_form routes for revisionable entity types. In other words: that is where the real root cause of @hchonov's concerns lies.
  3. Given that #2865616 already is in HEAD and has been for 2 months: "latest" revision" doesn't necessarily result in a revision with a usable translation for the Content Translation module. Which is why #2938895: Make EntityConverter load the latest translation-affecting revision for translated entities (just committed in the last 24 hours) changes the semantics of the load_latest_revision flag to load the latest translation-affecting revision for the current request's negotiated content language instead.
  4. In other words: #2938895 fixed the behavior introduced in #2865616 for multilingual sites.
  5. (And #2938895 then also set the load_latest_revision flag on all Content Translation routes, which was related, but secondary.)

Given that the November patch (#2865616) is the root cause of @hchonov's concerns, let's look at that patch a bit closer. Itmade two changes:

  1. made core's EntityConverter more versatile: made it support a flag that caused a different revision than the default to be loaded
  2. modified EntityResolverManager to add this flag by default to all entity form routes

The first change is not problematic because it is opt-in.
The second change is problematic because it is opt-out.

So @hchonov's concern is that the default we're now imposing on core favors Content Moderation at the cost of other contrib modules.

  1. If consensus agrees with that concern, then we should revert #2865616's changes to EntityResolverManager and #2938895's changes to ContentTranslationRoutes, keep the Content Moderation module's override logic and add logic to override Content Translation's routes. AKA "it sadly took 2 months to discover that regression, but at least it hasn't shipped in a stable release yet"
  2. If consensus disagrees with that concern, we can continue.

I personally do not feel qualified to say what the right approach is: I'm no expert on revisions nor translations nor content moderation nor Entity/Field API. I'm just trying to help out by hopefully painting a clearer picture of the situation.


I also did some digging into why this even uses a flag on the route definition. Why does this happen at the route parameter upcasting level? Why can't the desired revision be loaded during the preparing of the entity form? It was introduced in Content Moderation's contrib predecessor, workbench_moderation, in a commit without an issue/docs/rationale/tests. We just ended up first porting it to core and then generalizing it without questioning AFAICT. If we questioned it, then we at minimum failed to document the rationale.
Changing this now is a pretty invasive change … with its own set of BC concerns (because Content Moderation has beta stability since 8.4.0).


Hope this helps the discussion.

hchonov’s picture

Re #123
@Wim Leers++, really good summary!

Unfortunately I've missed the invasive change to the EntityResolverManager, and I think that this is wrong. I don't know why this decision has been made, but I don't feel comfortable with it.

If consensus agrees with that concern, then we should revert #2865616's changes to EntityResolverManager and #2938895's changes to ContentTranslationRoutes, keep the Content Moderation module's override logic and add logic to override Content Translation's routes. AKA "it sadly took 2 months to discover that regression, but at least it hasn't shipped in a stable release yet"

From my point of view this is the right option.

Changing this now is a pretty invasive change … with its own set of BC concerns (because Content Moderation has beta stability since 8.4.0).

We could revert the changes in the EntityResolverManager and content moderation could still do what the EntityResolverManager is doing so that content moderation behavior remains intact. But we should make it clear that content moderation definitely isn't compatible with other approaches relying on forward revisions.

I think that core should further use default revisions at all the places but offer the option to hook in and change behaviors regarding forward revisions.

hchonov’s picture

Additionally suddenly loading forward revisions instead of the default revisions might lead to data leak. We never know why forward revisions have been created and how they are used. To me this feels really dangerous.

Sam152’s picture

My question and the source of disagreement is: what are the expectations developers have with regards to how core entity forms behave when a pending revision is created? In the content_moderation use case, ignoring those pending revisions in entity forms is data loss. Are other (current or future) consumers of this api in the same boat or are there concrete use cases out there where this would mean data leaking instead of data being preserved? The entry point into the whole API doesn't strictly define exactly what "pending" or "forward" implies exactly.

Is it helpful to be more or less opinionated about this? Are we preventing headaches for someone with $usecase or making false assumptions which are equally tricky to undo. I think I'm biased towards the former simply based on content_moderation being the "flagship" consumer of this API. I think "latest" and "pending" have some implication towards "this is the point at which things progress" more than "this is a generalised place to store more information". Purely theoretical, but if we nudged integrators towards this paradigm, is there be more scope for things to play nicely together?

The flip side of all of this breaking BC really sucks. My belief is still that this should be treated as fixing a critical bug, furthering the efforts to make the latest revision a first class citizen however @hchonov makes great points that this should be carefully evaluated.

plach’s picture

@amateescu, #120:

1: Because that way it is more readable, easier to step through via the debugger, and can be type-hinted and get autocomplete. I can add more spaces of course :)
3: Yep, they are related: IIRC the problem was that if an entity is not translated it doesn't have a record corresponding the JOIN langcode in the data table and so the bundle cannot be determined. This was breaking tests. I believe the fix above is correct, I can dig in to try and figure out whether also the test change is correct, but since there was a @todo in the other test hinting that not everything was working as expected, I assumed also this test was not behaving properly as it was asserting the apparently "wrong" behavior.

I don't think this addressed #2927455: When adding a new translation the entity should start from the draft state explicitly, maybe as a positive side-effect ;)


@hchonov, #121:

[...] probably should somehow make it clear that content moderation could not be used together with other modules/approaches relying on forward revisions.

Well, I guess we were starting for this assumption, right? That there are modules whose use of pending revisions is fundamentally incompatible with how CM interprets them, right? I guess this will always be the case, at least until we introduce that "revision module owner/type" field we discussed in the past weeks.

Unfortunately this as well could not be the default behavior here, as it is only valid for the content moderation use case [...]

I'm not sure about that: if the entity has a translation in a pending revision (even an autosaved draft), it should be listed in the overview, otherwise how would you get access to it from the UI?


@Sam152, #122:

[...] I wonder if it's worth rolling the tests from there into here as well.

This patch is already rather big, could this be a follow-up?

[...] so maybe this is restoring some order.

Possibly, see reply above to @amateescu :)

On another (super painful) note, to 100% resolve the above issue for existing sites, we'd need an upgrade path to shuffle around which is the default content_moderation_state revision.

Are you sure about that? I think the fact that we have been blocking translated pending revisions on each other so far, should have prevented any data issue from arising. We need to merge cms revisions only now that we are unblocking that behavior, I think.


I will post my take on @hchonov's concerns in a following post.

hchonov’s picture

@plach:

Well, I guess we were starting for this assumption, right? That there are modules whose use of pending revisions is fundamentally incompatible with how CM interprets them, right? I guess this will always be the case, at least until we introduce that "revision module owner/type" field we discussed in the past weeks.

I thought it makes sense to point this out one more time :).

I'm not sure about that: if the entity has a translation in a pending revision (even an autosaved draft), it should be listed in the overview, otherwise how would you get access to it from the UI?

Actually not, the idea there is that user A should not have access to autosave revisions of user B. And there is not only a change in the overview but in the translation add functionality. Also there might be the use case that forward revisions are created and they have their own route where they are listed.

It is possible to isolate all those changes only to content moderation without having to go into a big and long discussion and start searching for use cases. If we could solve something without breaking BC, then why should we break BC?

plach’s picture

Back to the solutions to @hchonov's concerns I discussed with Wim (I'm starting from his summary in #123).

This is how I'd suggest to proceed:

  • Implement a variant of #115: make sure that latest revisions are handled the CM-way™ by Content Translation only when CM is enabled. At the same time enable the changes in the CT's codebase based on the same logic to address #121/#128. This should be enough to fix this issue.
  • Open a critical follow-up to do the same for all _entity_form routes.
  • Open another follow-up to finally discuss the definition of pending revision, how different behaviors can be reconciled or at least make sure they don't conflict with each other, and possibly re-enable latest revision in any entity form. Basically continue on the foundations laid by #126, which looks like a promising start. Since we were ready to perform this change in between 8.4 and 8.5, it should be ok to perform it in any minor, but I'm not sure we will be able to make that happen in 8.5.
  • Open an additional follow-up to re-evaulate the use of hook_entity_prepare_form(), instead of relying on routes, to integrate CM with entity forms. This would have the following advantages over the current approach:
    • it would support nested entity forms
    • it would support multiple unrelated entity forms on the same route

    OTOH it would have the drawback of making things less explicit.

plach’s picture

Component: entity system » content_moderation.module

Oh, btw, this is mainly a CM issue now :)

Sam152’s picture

On another (super painful) note, to 100% resolve the above issue for existing sites, we'd need an upgrade path to shuffle around which is the default content_moderation_state revision.

Are you sure about that? I think the fact that we have been blocking translated pending revisions on each other so far, should have prevented any data issue from arising. We need to merge cms revisions only now that we are unblocking that behavior, I think.

The related issue doesn't require translations to reproduce, but it is fixed by the cms revisions matching the default status of the host entity. Given the already big scope, lets follow up in the other issue with a fix for existing content or an appropriate work around. I think it's sufficiently edge-case to put to one side for now.

hchonov’s picture

Re #129.1:

Implement a variant of #115: make sure that latest revisions are handled the CM-way™ by Content Translation only when CM is enabled. At the same time enable the changes in the CT's codebase based on the same logic to address #121/#128. This should be enough to fix this issue.

I think that we should not place logic into the CT that is to be used only by content moderation. Instead we could offer a way to prepare the entity so that any module could hook in and make the necessary adjustments. Actually the changes in in CT::overview() and in CT::add() shouldn't be needed if content moderation has set the flag load_latest_revision on the corresponding routes, or is there something that I am missing?

plach’s picture

I think that we should not place logic into the CT that is to be used only by content moderation. Instead we could offer a way to prepare the entity so that any module could hook in and make the necessary adjustments.

That sounds premature to me: until we don't have a clear way forward on all the rest outlined above, introducing new APIs may be misguided. For instance, if we end up deciding the CM-way is the default core way, CT would be the right place for those changes. If we do it the way I'm suggesting, we leave the door open to moving the code to CM or opening it to any module leveraging pending revisions. The other way around may leave us with a lingering outdated API. Or were you thinking of an existing API?

Actually the changes in in CT::overview() and in CT::add() shouldn't be needed if content moderation has set the flag load_latest_revision on the corresponding routes, or is there something that I am missing?

Those are definitely needed anyway: the latest (translation-affecting) revision is not enough to determine all available translations in all revisions. And in the case of ::add() we want the latest revision affecting the source translation, so that needs to be adjusted as well.

Wim Leers’s picture

I like #129 a lot.

It sounds like @hchonov agrees with the general plan, with the sole exception that he'd like to see no if ($this->moduleHandler->moduleExists('content_moderation')) {…} statements inside the content_translation module. He'd like to see those in alters in the content_moderation module instead. I can see why he prefers that. But A) there are no such alter hook invocations in ContentTranslationController so there's no clean way to do this, B) that can definitely be done in a follow-up, C) is a should-have, not a must-have, since there is definitely no change in behavior while the Content Moderation module is not installed.

@hchonov: would you be okay with a follow-up to move the logic changes in content_translation into the Content Moderation module? That still allows this very important issue to proceed, and does so without locking ourselves into a particular behavior, thanks to you pointing that out! (And thanks again for your constructive criticism — I'm very glad you pointed this out before it was committed :))

EDIT: I wrote this while @plach was writing #133 apparently — apparently he's thinking pretty much the same.

plach’s picture

Wim Leers’s picture

Assigned: plach » catch
Status: Needs review » Reviewed & tested by the community
  • This got an RTBC from Content Moderation maintainer @timmillwood in #118
  • This got only 3 remarks from Field API maintainer @amateescu (and pretty much Workflow Initiative lead AFAICT) in #120, the first two of which were nits, the 3rd a question, and answered by @plach.
  • This got only 2 remarks from Workflows maintainer @Sam152 in #122, the second of which was more of an "also see", the first of which was a serious question
  • This got several remarks from Entity API maintainer @hchonov, with one pretty fundamental concern, which we've been discussing over the last few dozen comments, and summarized by yours truly in #123. Based on the discussion, there's a clear plan to move forward in #129.

Based on the general +1, I'm going to be bold and RTBC this, since it's generally supported by the relevant maintainers. #135 addresses @hchonov's immediate concern, his concern also applies to a patch from November, but we can and should do that in a follow-up.

I do think this should be reviewed & committed by @catch though, so assigning to him.

Note: @effulgentsia already is reverting the load_latest_revision flag in #2939247: Revert Content Translation routes to act on default rather than latest revisions.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -219,11 +219,11 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
-    if (!$entity->isNew() && !$entity->isDefaultRevision() && $entity->isTranslatable() && $this->isAnyRevisionTranslated($entity)) {
+    if (!$entity->isNew() && !$entity->isDefaultRevision() && $entity->isTranslatable() && ($entity->getTranslationLanguages(FALSE) || $this->isAnyRevisionTranslated($entity))) {

Minor, but out of curiosity, why is $entity->getTranslationLanguages(FALSE) being added here rather than within isAnyRevisionTranslated()? Because if the former is true, doesn't that mean that $entity itself has a translation, so shouldn't a method named isAnyRevisionTranslated() return true in that case?

hchonov’s picture

I think that we should not place logic into the CT that is to be used only by content moderation. Instead we could offer a way to prepare the entity so that any module could hook in and make the necessary adjustments.

That sounds premature to me: until we don't have a clear way forward on all the rest outlined above, introducing new APIs may be misguided. For instance, if we end up deciding the CM-way is the default core way, CT would be the right place for those changes. If we do it the way I'm suggesting, we leave the door open to moving the code to CM or opening it to any module leveraging pending revisions. The other way around may leave us with a lingering outdated API. Or were you thinking of an existing API?

+++ b/core/modules/content_translation/src/ContentTranslationManager.php
@@ -145,4 +145,19 @@ protected function loadContentLanguageSettings($entity_type_id, $bundle) {
+  public static function isPendingRevisionSupportEnabled() {
+    return \Drupal::moduleHandler()->moduleExists('content_moderation');
+  }

Isn't this actually a new API? Why is a new method needed instead of integrating this check directly into the code?

Or were you thinking of an existing API?

I don't think that we have any existing API. I was thinking that we could move the whole code regarding CM from CT to CM.

plach’s picture

@effulgentsia, #137:

Originally the method was called ::hasStoredTranslation() or something similar: it's checking stored revisions, so it does not consider newly added translations yet to be saved. You can see a hint about this in the PHP docblock. I agree this is potentially confusing now. What about renaming the method to ::isAnyStoredRevisionTranslated()? We could even add a new ::isAnyRevisionTranslated() then:

  protected function isAnyRevisionTranslated(TranslatableInterface $entity) {
    return $entity->getTranslationLanguages(FALSE) || $this->isAnyStoredRevisionTranslated($entity);
  }

@hchonov, #138:

Isn't this actually a new API? Why is a new method needed instead of integrating this check directly into the code?

It's an @internal method we can remove at any point in time, or we can expand into a full-fledged API.

I don't think that we have any existing API. I was thinking that we could move the whole code regarding CM from CT to CM.

The would mean either taking over the routes and duplicating a lot of code, or introducing new hooks. Both seem more complex than what #135 does. Unless you have a better idea, of course :)

catch’s picture

Assigned: catch » Unassigned

I'm +1 on #129, but haven't done a proper review of the code yet. Unassigning to leave open for review/commit if effulgentsia gets back to this before me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 135: entity-ml_drafts-2860097-135.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

Status: Needs work » Reviewed & tested by the community

Bot fluke

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

I just now committed #2939247: Revert Content Translation routes to act on default rather than latest revisions, so at a minimum, this will need a reroll, so NW for that.

Overall, I really like this patch a lot! Thank you to everyone who polished it so much to get it to this state.

I apologize if this is a pain, but I would like to see a couple small hunks split out into separate issues as follows:

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -60,6 +60,14 @@ function content_translation_module_implements_alter(&$implementations, $hook) {
    +    // Move our hook_entity_bundle_info_alter() implementation to the top of the
    +    // list, so that any other hook implementation can rely on bundles being
    +    // correctly marked as translatable.
    +    case 'entity_bundle_info_alter':
    +      $group = $implementations['content_translation'];
    +      $implementations = ['content_translation' => $group] + $implementations;
    +      break;
    

    I think just this much stands alone and is therefore worth committing alone. Can it be split out into a separate issue, with a test added?

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -422,6 +430,11 @@ function content_translation_form_field_config_edit_form_alter(array &$form, For
    +    if (!$manager->isEnabled($entity->getEntityTypeId(), $entity->bundle())) {
    +      return;
    +    }
    

    I can't tell if this is fixing an obvious bug or if it's a subtle behavior change that might have unintended consequences. Can this be split into its own issue with an explanation on that issue as to whether it's fixing a bug for which we don't have test coverage (in which case we need a test added) or with some other explanation as to why it's the desired behavior?

  3. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
    @@ -145,4 +145,19 @@ protected function loadContentLanguageSettings($entity_type_id, $bundle) {
    +  public static function isPendingRevisionSupportEnabled() {
    

    Given #138, can this (and all places that call it) be moved into its own issue (either with the implementation it currently has in this patch, or simply returning FALSE in all cases, if we don't want that issue changing core behavior yet until the rest of this issue is committed)? I'm +1 to this approach, but I'd like to commit it on its own, and have a dedicated issue in which to continue the discussion about it and manage followups from.

  4. +++ b/core/modules/node/src/NodeViewBuilder.php
    @@ -89,7 +89,9 @@ public static function renderLinks($node_entity_id, $view_mode, $langcode, $is_i
    -      $entity = Node::load($node_entity_id)->getTranslation($langcode);
    +      /** @var \Drupal\node\NodeInterface $entity */
    +      $entity = \Drupal::service('entity.repository')
    +        ->getTranslationFromContext(Node::load($node_entity_id), $langcode);
    

    This also looks like a standalone fix to me. Can it be moved to a separate issue as well? Is there a case where it's a useful change even without CM enabled? If so, can we add a corresponding test?

effulgentsia’s picture

  1. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -268,6 +270,40 @@ public function entityBaseFieldInfo(EntityTypeInterface $entity_type) {
    +      // Restore the revision ID as other modules may expect to find it still
    +      // populated. This will reset the "new revision" flag, however the entity
    +      // object wil be marked as a new revision again on presave.
    +      // @see \Drupal\content_moderation\Entity\Handler\ModerationHandler::onPresave()
    

    This seems fragile, but I don't have a better idea, so ok.

  2. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -35,11 +35,6 @@ public function onPresave(ContentEntityInterface $entity, $default_revision, $pu
         // This is probably not necessary if configuration is setup correctly.
         $entity->setNewRevision(TRUE);
    

    Given the above though, this comment needs updating.

effulgentsia’s picture

diff --git a/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php

Also, I just now committed #2924812: Deprecate EntityRevisionConverter in content_moderation., so I think all changes to this file can now be removed from this patch?

effulgentsia’s picture

+++ b/core/modules/content_moderation/content_moderation.module
@@ -251,10 +261,16 @@ function content_moderation_entity_bundle_info_alter(&$bundles) {
+          // If we have even one moderation-enabled translatable bundle, we need
+          // to make the moderation state bundle translatable as well.
+          if (!empty($bundles[$entity_type_id][$bundle_id]['translatable'])) {
+            $translatable = TRUE;
+          }

Can we add a phrase or sentence to the comment explaining why that's needed? Perhaps something like "If we have even one moderation-enabled translatable bundle, we need to make the moderation state bundle translatable as well, to allow each translation to be moderated independently."? Feel free to improve that to be more accurate or specific :)

plach’s picture

Title: Ensure that content translations can be moderated independently » [PP-4] Ensure that content translations can be moderated independently
Status: Needs work » Needs review
FileSize
1.99 KB
71.8 KB
54.95 KB

@effulgentsia, #143:

1: Created #2939909: Ensure that hook_bundle_info_alter() implementations know whether bundles are translatable for that.
2: Created #2939917: Field synchronization should run only for Content Translation-enabled entities for that.
3: I don't think it is viable to do this, sorry: the logic that conditionally enables latest revision support in CT needs to be in this patch, because if it goes in earlier there would be no perceived change, since language-independent moderation would still be blocked, and something may even break, because there would be no sensible test coverage we could add. OTOH nothing will work properly here without those changes, and we’d have to rework or comment out test coverage just the get a green patch.
4: Created #2939742: Node links always load the default revision during rendering for that.

I also created #2939795: Multilingual logic is not applied when a new revision translation is being added to take care of #137/#139.


#144:

I have verified that CM relies on the core behavior in ContentEntityForm::buildEntity() to set the new revision flag, so the current approach should be perfectly safe. Updated the comment to reflect that.


#145, #146:

Done :)


Once again attaching a review patch and a combined patch, since now this depends on the four issues above.

plach’s picture

Issue summary: View changes

Updated IS

plach’s picture

Issue summary: View changes
plach’s picture

Title: [PP-4] Ensure that content translations can be moderated independently » [PP-2] Ensure that content translations can be moderated independently
Wim Leers’s picture

Title: [PP-2] Ensure that content translations can be moderated independently » [PP-1] Ensure that content translations can be moderated independently
effulgentsia’s picture

I committed #2939917: Field synchronization should run only for Content Translation-enabled entities. This is a re-upload of #147's ".review" patch.

Re #147.3: ok, that's fine. It means this patch has changes to content_moderation and content_translation modules, but only those two now, which is great! Thank you very much for splitting out the others and adding CM-independent test coverage to them.

matsbla’s picture

I tested now on last dev version of 8.5.x to see what happens when I try to create a pending revision for a translatable image field containing a non-translatable image file. What happens is that when I change the non-translatable image file and save as draft the image file for published source language is also updated with the new image file from draft version (but still the old image and title texts).

If the non-translatable image files are edited concurrently on different translations it can lead to there being different image files for different language version because the lock is not working #2912318: Changing field value in an entity presave hook will not update the entity changed timestamp.

Maybe it would make sense to not make it possible to edit the untranslatable image files in pending revisions?

Status: Needs review » Needs work

The last submitted patch, 152: entity-ml_drafts-2860097-152.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

effulgentsia’s picture

@matsbla: thanks for testing. Are you able to reproduce the same or similar bug with any field other than a file or image field?

matsbla’s picture

No, but I do not know about any other fields than the Image field that can be translatable but still have non-translatable attributes.
If you have suggestions for such fields I can check it.

For the file field there is a description attribute but doesn't look like it is possible to keep the file non-translatable if you want to add translations to the description. So either both or none of them is translatable (at least from the UI).

plach’s picture

Applied the code sniffer patch from #154.

@matsbla, #153 / #156:

Right, we forgot to open a follow-up issue as suggested in #2878556-17: Ensure that changes to untranslatable fields affect only one translation in pending revisions.

To sum up: we would like to see translatable fields having field synchronization enabled behave as untranslatable fields, because they may introduce the same issues that untranslatable fields would have caused if we didn't do #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions.

There's nothing in the current patch that is wrong with respect to that issue, it just enables the UI changes that allow to reproduce it. For this reason I think the problem reported above should be handled in a separate issue. I'm not sure whether the latter needs to be a blocker or may be a follow-up, but I think that this change can only happen before beta, while that fix could happen in any patch release, since that's just a bug fix.

For now I just filed the separate issue: #2940204: Translatable fields with synchronization enabled should behave as untranslatable fields with respect to pending revisions.

plach’s picture

The more I think about it and the more I lean towards making #2940204: Translatable fields with synchronization enabled should behave as untranslatable fields with respect to pending revisions a blocker for this. I'm wondering whether it would be acceptable to implement a stop-gap fix there to finally unblock this issue and then work on a proper solution that may or may not make it into 8.5.0.

hass’s picture

This issue here is the most important left issue I can think of (next to content moderation). Not having this bug here fixed makes the whole translation system not usable. I really hope 8.5 becomes the very first useable D8 release and not 8.6. Whatever happens, this one need to go in before final 8.5.

@plach: Thanks for working on this issues. You are hero of the year.

Sam152’s picture

I've opened #2940575: Document the scope and purpose of pending revisions to focus the discussion that took place at #123 to #128.

matsbla’s picture

I tested patch from #157 and to me it seems like after I apply it I'm not any longer able to delete drafts for languages that didn't yet have any published translations. I tested both using the "delete" operation in Translation tab, the "delete" button in the draft form, and the "delete" button in the local task bar. Every time I get the status message like: [French] translation of the Article [blablabla] has been deleted. But when I go back to the translation tab again the draft is still there.

Maybe it should be possible to discard a draft without deleting the last published version of the language. Right now it doesn't seem like we do have that choice.

Wim Leers’s picture

@plach asked me to create a follow-up for #162, which he says is a known limitation, due to a pre-existing limitation in underlying Entity/Field API infrastructure: #2940890: Don’t allow deleting revision translations in pending revisions.

Wim Leers’s picture

Created all follow-ups that @plach suggested/requested in #129:

  • Open a critical follow-up to do the same for all _entity_form routes.

Done: #2940899: Regression: _entity_form routes should not get load_latest_revision_flag by default: changes default behavior (= regression) while only Content Moderation needs it.

  • Open another follow-up to finally discuss the definition of pending revision, how different behaviors can be reconciled or at least make sure they don't conflict with each other, and possibly re-enable latest revision in any entity form. Basically continue on the foundations laid by #126, which looks like a promising start. Since we were ready to perform this change in between 8.4 and 8.5, it should be ok to perform it in any minor, but I'm not sure we will be able to make that happen in 8.5.

Done: #2940904: Settle on definition of "pending revision".

  • Open an additional follow-up to re-evaulate the use of hook_entity_prepare_form(), instead of relying on routes, to integrate CM with entity forms. This would have the following advantages over the current approach:
    • it would support nested entity forms
    • it would support multiple unrelated entity forms on the same route

    OTOH it would have the drawback of making things less explicit.

Done: #2940907: Consider using hook_entity_prepare_form() rather than routes to integrate Content Moderation with entity forms.

Wim Leers’s picture

Oops, #2940904: Settle on definition of "pending revision" is a duplicate of #2940575: Document the scope and purpose of pending revisions, which @Sam152 already created. 😊 Fortunately, it wasn't a waste of time at all, since the IS of Sam's issue was sparse, mine provided more context. So, closed mine, moved my IS to Sam's, all good! 👍

Sam152’s picture

Thanks @Wim!

+++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -176,6 +171,12 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
+      $content_moderation_state = $storage->createRevision($content_moderation_state, $entity->isDefaultRevision());

The more I think about this, the more I kind of think we shouldn't change it in this patch if we can at all help it. How much of the current approach depends on this?

If we can validate that the new behavior in the views integration is indeed correct after this change, old content would still behave differently to new content. The current behavior is: any pending revisions of a content entity do not create pending revisions of the ContentModerationState entity. That's the case regardless of translations and after this patch gets in, the defaultness of all your existing ContentModerationState entities doesn't change. Ie, I could have a lot of default CMS entity revisions set as 'draft', associated with their content entity counterparts whose default revision is 'published'.

The views stuff is less important imo, given the computed field now covers all use cases of the join, however what happens if something is introduced which is important and does depend on this change? We'd could be introducing some weird behavior for any existing items of content.

If the answer is: it just isn't that risky, that's all good with me. It's just the only part left of this I'm ambivalent about and it seems this is close to going in!

effulgentsia’s picture

@Sam152, @plach: I opened #2941736: Moderation state revisions should have their isDefaultRevision() match the host entity's. Is that the correct solution to #166? If so, do we need any test coverage there other than what is in that issue summary's reproduction steps?

plach’s picture

Title: Ensure that content translations can be moderated independently » [PP-1] Ensure that content translations can be moderated independently
Issue summary: View changes
Status: Needs review » Postponed

Postponed once again...

plach’s picture

plach’s picture

Status: Postponed » Needs review
plach’s picture

Title: [PP-1] Ensure that content translations can be moderated independently » Ensure that content translations can be moderated independently
Status: Needs review » Reviewed & tested by the community

#2941736: Moderation state revisions should have their isDefaultRevision() match the host entity's has been committed, moving back to RTBC as all the pending concerns have been resolved!

amateescu’s picture

  1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -221,4 +221,16 @@ protected function realSave() {
    +    // We need to skip the parent entity revision ID, since that will always
    +    // change on every save, otherwise every translation would be marked as
    +    // affected regardless of actual changes.
    +    $field_names[] = 'content_entity_revision_id';
    

    Why do we need to explicitly skip this field? It is not translatable so it shouldn't be taken into consideration in the first place, no?

  2. +++ b/core/modules/content_moderation/src/Form/EntityModerationForm.php
    @@ -138,6 +138,9 @@ public function buildForm(array $form, FormStateInterface $form_state, ContentEn
    +    /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
    +    $storage = \Drupal::entityTypeManager()->getStorage($entity->getEntityTypeId());
    +    $entity = $storage->createRevision($entity, $entity->isDefaultRevision());
    

    These objects doesn't seem to be used anywhere.

  3. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -145,8 +138,19 @@ public function isLatestRevision(ContentEntityInterface $entity) {
    +      $default_revision_id = $this->getDefaultRevisionId($entity->getEntityTypeId(), $entity->id());
    

    Can we first check if the passed-in $entity is not the default revision already? It would save an entity query :)

  4. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -145,8 +138,19 @@ public function isLatestRevision(ContentEntityInterface $entity) {
    +      if ($latest_revision_id !== $default_revision_id) {
    

    We shouldn't use strict equality checks on field values.

  5. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
    @@ -145,4 +145,19 @@ protected function loadContentLanguageSettings($entity_type_id, $bundle) {
    +   * @internal
    +   *   There is ongoing discussion about how pending revisions should behave.
    +   *   Content Translation for now is compatible with Content Moderation, so we
    +   *   we enable pending revision support only if it is enabled.
    

    I think we should add a @todo pointing to #2940575: Document the scope and purpose of pending revisions. That issue should at least remove the "for now" part of this comment or rewrite it if needed.

  6. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -99,6 +101,9 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
    +    $default_revision = $entity;
    
    @@ -121,6 +126,16 @@ public function overview(RouteMatchInterface $route_match, $entity_type_id = NUL
    +          $entity = $latest_revision_id ? $storage->loadRevision($latest_revision_id) : $default_revision;
    

    Why do we need to instantiate the current $entity object to a different variable, only to put it back a few lines below?

plach’s picture

This should address #172.

A few replies:

1: Because also changes to untranslatable fields are considered when determining the value of the RTA flag. We need to take that into account otherwise we run into the situation described by the comment.
2: Well, $storage is used to create the new revision, $entity is used in all the following lines. Am I missing something? :)
3: Good point. I had to add a check to make sure that we are not dealing with an $entity object that has just been marked as the new default revision.
4: Fixed
5: Rewrote a comment and add a @see linking to the issue, as we're not sure yet what consequences the discussion will have on that bit of code, although it's likely to be revisited.
6: Because the second bit is in a loop: for each language there may not be a latest translation-affecting revision, in which case we always have to fall back to the default revision.

amateescu’s picture

Ok, no other remarks from me :) RTBC++

effulgentsia’s picture

Thanks for all the great work on this. The patch looks great to me. Ticking credit boxes for reviewers and testers.

effulgentsia’s picture

+++ b/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php
@@ -191,4 +202,358 @@ public function testTranslateModeratedContent() {
+    $message = $is_new ? t('Article ' . $title . ' has been created.') : t('Article ' . $title . ' has been updated.');

This failed PHPCS, because of concatenating within t(). If we need to call t() here, then we should match the string in NodeForm::save(), which would be t('@type %title has been created.'), since that would be the only one for which we'd have a translation. However, most of our tests, including this one, don't run with a different interface language, so I think we can just remove the t(), so that's what I did in this patch.

  • effulgentsia committed a261673 on 8.6.x
    Issue #2860097 by plach, timmillwood, Sam152, Wim Leers, hchonov, catch...

  • effulgentsia committed f50cc65 on 8.5.x
    Issue #2860097 by plach, timmillwood, Sam152, Wim Leers, hchonov, catch...
effulgentsia’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +8.5.0 highlights

Pushed to 8.6.x and 8.5.x!

If anyone disagrees with the interdiff in #176, please open a followup to correct it. Thanks.

See you all in the followups. I'm especially interested/hopeful in getting #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key resolved before beta. I think all the others are ok to either land after beta, or in 8.6 only, but please correct me if I'm wrong on that.

Status: Fixed » Closed (fixed)

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