"Create new revision" option is ignored when updating EntityReferenceRevisionsItem

After some investigation I figured out this is due to the "revision_translation_affected" field being not set, when you just edit the inner paragraph.

 select * from node_field_revision;
+-----+-----+----------+--------+-------+-----+------------+------------+---------+--------+------------------+-------------------------------+
| nid | vid | langcode | status | title | uid | created    | changed    | promote | sticky | default_langcode | revision_translation_affected |
+-----+-----+----------+--------+-------+-----+------------+------------+---------+--------+------------------+-------------------------------+
|   1 |   1 | en       |      1 | test  |   1 | 1548716108 | 1548716137 |       0 |      0 |                1 |                             1 |
|   1 |   2 | en       |      1 | test2 |   1 | 1548716108 | 1548716158 |       0 |      0 |                1 |                             1 |
|   1 |   3 | en       |      1 | test2 |   1 | 1548716108 | 1548716572 |       0 |      0 |                1 |                          NULL |
|   1 |   4 | en       |      1 | test2 |   1 | 1548716108 | 1548749830 |       0 |      0 |                1 |                          NULL |
|   1 |   5 | en       |      1 | test2 |   1 | 1548716108 | 1548749849 |       0 |      0 |                1 |                          NULL |
|   1 |   6 | en       |      1 | test2 |   1 | 1548716108 | 1548749904 |       0 |      0 |                1 |                          NULL |
|   1 |   7 | en       |      1 | test2 |   1 | 1548716108 | 1548749976 |       0 |      0 |                1 |                          NULL |
|   1 |   8 | en       |      1 | test2 |   1 | 1548716108 | 1548749995 |       0 |      0 |                1 |                          NULL |
|   1 |   9 | en       |      1 | test3 |   1 | 1548716108 | 1548750110 |       0 |      0 |                1 |                             1 |
|   1 |  10 | en       |      1 | test3 |   1 | 1548716108 | 1548750165 |       0 |      0 |                1 |                          NULL |
|   1 |  11 | en       |      1 | test3 |   1 | 1548716108 | 1548750210 |       0 |      0 |                1 |                          NULL |
|   1 |  12 | en       |      1 | test3 |   1 | 1548716108 | 1548750284 |       0 |      0 |                1 |                          NULL |
|   1 |  13 | en       |      1 | test3 |   1 | 1548716108 | 1548750372 |       0 |      0 |                1 |                          NULL |
|   1 |  14 | en       |      1 | test3 |   1 | 1548716108 | 1548750603 |       0 |      0 |                1 |                          NULL |
|   1 |  15 | en       |      1 | test4 |   1 | 1548716108 | 1548751142 |       0 |      0 |                1 |                             1 |
+-----+-----+----------+--------+-------+-----+------------+------------+---------+--------+------------------+-------------------------------+

Enforcing this in entity_reference_revisions makes it working, I'm not fully convinced though given the amount of complexity which is in this entire problem space. I've not yet written a test, but given the clear instructions above it feels like this is something which can be done with a reasonable effort.

+1 same issue

@miro_dietiker, is it worth fixing?

#3 fixed it for me, thank you very much for the patch! :-)
This issue actually broke one of our projects and from my perspective it is very important that we add this to the next release of the module.

#3 working for me.

#3 is working for me, too. What steps are necessary to get this committed?

#3 works fine but it fails testing.

Attached is an updated patch that should pass testing. It adds a check that the host has translation changes before setting the revision_translation_affected flag to true.

#13 works for me. Tested as follows on a site that does not use translation on an existing node containing a paragraphs field:

1. Changed the title and saved with a new revision. A new revision was created as expected.
2. Changed the content of a text field within the paragraphs field and saved with a new revision. New revision is missing from the node's revision list.
3. Applied patch from #13
4. Repeated steps 1 and 2 and observed proper new revision behavior on the node in both cases.

#3 works for me but #13 doesn't. With the translation check included in #13 patch, I don't see revisions recorded unless I change the node title.

same here. #3 works, #13 doesnt. Thanks!

I am going to try to write a test for this issue. Assigning to myself.

#3 seems to be a correct patch.

Review of #13:

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -283,6 +283,11 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+        if ($host instanceof TranslatableRevisionableInterface && $host->hasTranslationChanges()) {
+          $host->setRevisionTranslationAffected(TRUE);
+        }

I think instanceof check is not necessary as Drupal\Core\Entity\ContentEntityBase::setRevisionTranslationAffected() has already a similar check.
Also, the other condition ($host->hasTranslationChanges()) means that we mark a host entity revision as affected only if a user has made some changes in the host entity itself. That said, the use-case described in the issue description wouldn't be covered.

I am uploading a patch that provides a kernel test for the steps written in the issue summary:

Steps to reproduce:

1. Create new Paragraph type. Add Text field.
2. Create new Node type. Add Paragraph reference field.
3. Create new Node content. Add at least one paragraph. Save.
4. Edit the node. Change the title, check the "Create new revision" checkbox, add revision log message. Save.
5. Go to revision tab and observe the new revision was in fact created.
6. Go back to node edit form. This time modify the field inside the nested paragraph, then check the checkbox and add log message as previously. Save.
7. Go to revision tab and see the new revision is missing.

I assume this use-case is a valid one and this is how the module should work - update the host entity revision's revision_translation_affected field if there are changes in the referenced ERR entity.
However, I am not 100% sure if Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::preSave() is the best place to fix it.

I would expect that this use-case is recognized in Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList::hasAffectingChanges() which would tell the host entity there are actual changes. If so, this brings us to #3017718: New Updates / Revisions dont work anymore for components and the patch from #3017718.7 which fixes our use-case too. #3017718.13 points out this was intentionally done which makes it much harder to solve...

That said, #3025709: "Create new revision" option is ignored when updating EntityReferenceRevisionsItem looks like a duplicate of #3017718: New Updates / Revisions dont work anymore for components.

Hi,
Just wanted to report results of testing patch from #19 in a multilingual site.

The patch fixes the problem about changes made in a Paragraph (Entity Reference Revisions field) in the default (en) language, but oddly enough when changes are made on a translated node > paragraph:

1. the revision is not created for the translated node
2. a new revision is created for the default language node and is set as the "default revision" (althou checking with diff module, shows no differences with prev revision: "No visible changes")

I've checked the DB and it seems like the revision tables are correctly created for the paragraph field for the translated node too?

`paragraph_revision__field_about_heading` table
+---------------+-----------+-------------+----------+------------------------------------+
| bundle        | entity_id | revision_id | langcode | field_about_heading_value          |
+---------------+-----------+-------------+----------+------------------------------------+
| about_banner  |        92 |         622 | en       | Banner change value in english node|
| about_banner  |        92 |         622 | es       | Cambio en el campo bannerodo en ES |
+---------------+-----------+-------------+----------+------------------------------------+

The other table I've checked is `node_field_revision` to see the revision_translation_affected field:

table excerpt  
+---+---+------+----------------+-----------------------------+--------------------------+
|nid|vid|title |default_langcode|revision_translation_affected|content_translation_source|
+---+---+------+----------------+-----------------------------+--------------------------+
|  5| 36|Acerca|              0 |                        NULL | en                       |
+---+---+------+----------------+-----------------------------+--------------------------+

When editing paragraphs field in Spanish node, the "revision_translation_affected" is always set to NULL.

Let me know if you need more info,

Cheers,

m

After comparing both discussions here and in #3017718: New Updates / Revisions dont work anymore for components, there is two issues that needs to be solved for which I am uploading a patch for.

I have implemented two new tests, one for the issue described in the issue summary and one related to the issue raised in #22 which is effectively the same issue but for when you edit a ERR entity in non default translation

Although I think this should be handled in Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList::hasAffectingChanges(), because Drupal\Core\Entity\ContentEntityBase::hasTranslationChanges() does not check for hasAffectingChanges on field items that are not translatable, hidden on translation forms and not in the default translation, this has to be handled by Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem on preSave by setting setRevisionTranslationAffected on host translations that holds a reference that has been changed.

I can confirm that patch #23 fixes both cases described.

I am uploading a patch that adds a comment why we are making the changes in Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::preSave() and fixes code style issues.

Also, adding a related core issue #2769741: Node revisions page might not list a default revision that describes a use-case when a default revision might be "missing" in the user interface.

Replaced deprecated usage of node_load and small fixes to the tests.

+++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTranslationTest.php
@@ -439,6 +439,67 @@ class EntityReferenceRevisionsCompositeTranslationTest extends EntityKernelTestB
+
+    // Test that changing composite non default language (DE) reference results
+    // in translation changes for this language but not for the default
+    // language.
+    $node_de->get('composite_reference')->entity->getTranslation('de')->set('name', 'Change Composite #1 DE');
+    $node_de->setNewRevision();
+    $node_de->save();

The problem now seems to be that the opposite should also be true, but isn't yet, the translation is always marked as affected too. See updated patch.

wrong patch.

Good catch! I meant to use hasTranslationChanges() instead of isRevisionTranslationAffected()

Also made me realize that the number of revisions of the EN translation not revision translation affected should be 2 as the test is first translating the node (and the composite entity) in DE that should not affect the default language, and then it changes the composite entity in DE as well which should again not affect the EN revisions.

I have also updated the assertion messages as some were wrong

AFAIT the tests failed in #28 because $this->entity->getTranslation($langcode)->isRevisionTranslationAffected() condition reports true on each composite entity translation.

I am not sure how the revision_translation_affected flag is calculated on the composite entity (I tested with Paragraphs), but I assume it's because in my configuration Paragraph/ERR field is not translatable. Replaced that with $this->entity->getTranslation($langcode)->hasTranslationChanges() condition which should be more precise but more costly.

Also, updated tests to count affected translation revisions instead of null values.

@NiCo_O sorry about the cross post. :) It looks like #30 attached an older patch?

@mbovan no problem, you basically came up to the same conclusion and I got the tests wrong, so I think we are now good to go with your patch

Yeah, looks like we need to coordinate who's working on it in case more changes are necessary, I suggest if someone works on it then that person should assign the issue to himself and unassign when done.

Note that we're also testing this against the paragraph tests, as that has a lot of extra test coverage that also caught this problem, in #3064511: ERR Patch testing.

To all the people following this issue, it would be extremely valuable to test the latest patch and provide feedback if you see any problems/regressions.

The Paragraphs tests in #3064511-4 are green now. ✅

I tested the patch from #31 against a site that does not use translation. It does correct the problem of creating a new revision when a change is made to paragraphs field. Tested as follows, starting with an unpatched entity_reference_revisions 8.x-1.6:

1. Changed the title and saved with a new revision; observed proper new revision behavior.
2. Changed the content of a text field within the paragraphs field and saved with a new revision. No new revision is shown on the node's revision page.
3. Applied the patch from #31.
4. Repeated steps 1 and 2 and observed proper new revision behavior on the node in both cases.

So, it looks good for my case, but I'm sure it would be helpful to hear the results of someone using translation.

Hi, I've tested #31 in multilingual context and changes in "first level" paragraphs are triggering revisions in both languages.
BUT changes made inside a "nested" paragraph only triggers revisions in the Default language. For the translated node, no revisions appear in the revisions page.

Attaching here some screenshots and a video demonstrating the manual test.

As far as I can see, for the "child" paragraph in the translated node (es) the revision_translation_affected field in node_field_revision table remains in "null", whereas for the "parent" paragrpahs get's correctly set to "1".
Screenshot:

Just for a quick check, I've manually changed that "null" value to "1" and I could see the revision in the revisions page in ES.

Setting status back to "needs work" since I doubt that's the expected behaviour :S

Thanks!

m

I can confirm that the patch in #31 also works.

Testing #31 on my monolingual dev site:

It seems to work great in the following instances:
Text field in a paragraph.
Boolean field in a paragraph.
Physical field in a paragraph (i.e. https://www.drupal.org/project/physical) - both number and unit fields.

I notice that previous revisions on these fields before patch #31 don't show up on the revisions table - I'm assuming these revisions are gone for good?

Patch in #31 works for me too.

I can confirm patch from #31 works as well. I'm also using it in a multilingual context, with paragraphs and child (nested) paragraphs and have not had any issues with revisions being tracked.

Interesting... @mikemiles86 are you using core content_moderation module?
If that's not the case I will re-test patch from #31 in multilingual context

Are you using no moderation module or are you using workbench_moderation? If the second, there is likely little we can do to fully support that, we worked to add multiple features that we need to Drupal 8.6 so we could make multilingual drafts and publishing them work with the core content_moderation module.

Not using a moderation/workflow module at all should work too, as you don't have a UI to create drafts.

Since several people have now reported that it is working for them, including nested paragraphs and it is definitely a massive improvement, I'm considering to commit this and if you still have problems (without workbench_moderation), we/you can create a new issue about that.

Hi, I've tested patch in #31 without any moderation, and those are the results I've provided in my prev comment #38

I was asking about moderation because I thought that maybe using core content_moderation would resolve the 'nested' paragraphs issue I've described?
In the manual tests I did, it was the only scenario still failing after applying patch.

I agree this is a huge improvement! I just think that multilingual context and "nested" paragraphs are pretty common setups... but I guess we can create a separate ticket to address that.

I could reproduce the problem @anairamzap described in #38 and #46

I have tried to write a test for it (attached patch) but unfortunately, it does not fail as expected...

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -282,6 +282,24 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+            if ($this->entity->hasTranslation($langcode) && $this->entity->getTranslation($langcode)->hasTranslationChanges() && $this->target_revision_id != $this->entity->getRevisionId()) {
+              $translation->setRevisionTranslationAffected(TRUE);
+              $translation->setRevisionTranslationAffectedEnforced(TRUE);
+            }

Regarding the described problem, as far as I could tell $this->entity->getTranslation($langcode)->hasTranslationChanges() does not report TRUE when it is called on nested/parent entity.

Later in the execution, $this->entity->getTranslation($langcode)->hasTranslationChanges() reports TRUE when it is called on a child entity. The same code above correctly updates affected revision translation flags but *only* on the parent entity and not on the top host entity (e.g. node).

One idea would be to try to get all parent translations and update their affected revision translation flags if we end up with translation changes for a child entity.

Removed Paragraph class leftover from #47

@mbovan: We had this problem and after applying your patch from comment #48, revisions were being saved again. Somebody else from our team double-checked your code and it looks okay.

3025709-48.patch worked for me.
New node revisions were created when only a paragraph field was changed.

After much pondering, I've decided to commit this as an incremental improvement that already helps a lot of people. I've created #3085967: Resolve additional issues with detecting affected revision status as a follow-up for more advanced use cases.

So, new release is coming, right?

Yes. It has been fixed in version 1.7 on the 5th of October, 2019