Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Regression caused by #2961399: Support parallel translation forward revisions on untranslatable fields
Problem/Motivation
TL;DR The latest release of entity_reference_revisions (1.6.0) breaks the expected behaviour of paragraps-enabled fields.
Steps to reproduce:
- Create new Paragraph type. Add Text field.
- Create new Node type. Add Paragraph reference field.
- Create new Node content. Add at least one paragraph. Save.
- Edit the node. Change the title, check the "Create new revision" checkbox, add revision log message. Save.
- Go to revision tab and observe the new revision was in fact created.
- 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.
- Go to revision tab and see the new revision is missing.
Please note that the new revision of the paragraph was in fact created and referenced via paragraph reference field on node (entity_reference_revision field) but the revision of the node has been updated rather than creating new node revision.
Marking as Critical as it causes loss/corruption of stored data.
Comment | File | Size | Author |
---|---|---|---|
#48 | 3025709-48-31-interdiff.txt | 4.58 KB | mbovan |
#48 | 3025709-48.patch | 12.58 KB | mbovan |
| |||
#38 | test_3025709-31_err_multilingual.mp4 | 18.11 MB | anairamzap |
#38 | node_field_revision_table.png | 64.64 KB | anairamzap |
#31 | 3025709-30-28-interdiff.txt | 6.07 KB | mbovan |
Comments
Comment #2
rrrob CreditAttribution: rrrob at Chapter Three commentedComment #3
dawehnerAfter some investigation I figured out this is due to the "revision_translation_affected" field being not set, when you just edit the inner paragraph.
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.Comment #4
charly71 CreditAttribution: charly71 commented+1 same issue
Comment #5
maximpodorov CreditAttribution: maximpodorov commented@miro_dietiker, is it worth fixing?
Comment #6
sgurlt CreditAttribution: sgurlt as a volunteer and at Bright Solutions GmbH commented#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.
Comment #8
JeroenTComment #10
danthorne#3 working for me.
Comment #11
daveiano#3 is working for me, too. What steps are necessary to get this committed?
Comment #12
JvE CreditAttribution: JvE at One Shoe commentedComment #13
klaasvw CreditAttribution: klaasvw at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented#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.
Comment #14
tmanhollan CreditAttribution: tmanhollan commented#13 works for me. Tested as follows on a site that does not use translation on an existing node containing a paragraphs field:
Comment #16
mrogers CreditAttribution: mrogers commented#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.
Comment #17
el1_1el CreditAttribution: el1_1el commentedsame here. #3 works, #13 doesnt. Thanks!
Comment #18
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI am going to try to write a test for this issue. Assigning to myself.
#3 seems to be a correct patch.
Review of #13:
I think
instanceof
check is not necessary asDrupal\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.Comment #19
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI am uploading a patch that provides a kernel test for the steps written in the issue summary:
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.
Comment #22
anairamzapHi,
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:
I've checked the DB and it seems like the revision tables are correctly created for the paragraph field for the translated node too?
The other table I've checked is `node_field_revision` to see the revision_translation_affected field:
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
Comment #23
NiCo_O CreditAttribution: NiCo_O at Acquia Site Studio commentedAfter 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()
, becauseDrupal\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 byDrupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem
on preSave by setting setRevisionTranslationAffected on host translations that holds a reference that has been changed.Comment #24
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI 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.
Comment #26
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedReplaced deprecated usage of
node_load
and small fixes to the tests.Comment #27
BerdirThe 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.
Comment #28
Berdirwrong patch.
Comment #30
NiCo_O CreditAttribution: NiCo_O at Acquia Site Studio commentedGood catch! I meant to use
hasTranslationChanges()
instead ofisRevisionTranslationAffected()
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
Comment #31
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedAFAIT the tests failed in #28 because
$this->entity->getTranslation($langcode)->isRevisionTranslationAffected()
condition reportstrue
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.Comment #33
mbovan CreditAttribution: mbovan at MD Systems GmbH commented@NiCo_O sorry about the cross post. :) It looks like #30 attached an older patch?
Comment #34
NiCo_O CreditAttribution: NiCo_O at Acquia Site Studio commented@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
Comment #35
BerdirYeah, 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.
Comment #36
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedThe Paragraphs tests in #3064511-4 are green now. ✅
Comment #37
tmanhollan CreditAttribution: tmanhollan commentedI 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:
So, it looks good for my case, but I'm sure it would be helpful to hear the results of someone using translation.
Comment #38
anairamzapHi, 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 innode_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
Comment #39
StevenPatzI can confirm that the patch in #31 also works.
Comment #40
TrevorBradley CreditAttribution: TrevorBradley commentedTesting #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?
Comment #41
RenrhafPatch in #31 works for me too.
Comment #42
mikemiles86I 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.
Comment #43
anairamzapInteresting... @mikemiles86 are you using core content_moderation module?
If that's not the case I will re-test patch from #31 in multilingual context
Comment #44
BerdirAre 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.
Comment #45
BerdirSince 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.
Comment #46
anairamzapHi, 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.
Comment #47
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI 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...
Regarding the described problem, as far as I could tell
$this->entity->getTranslation($langcode)->hasTranslationChanges()
does not reportTRUE
when it is called on nested/parent entity.Later in the execution,
$this->entity->getTranslation($langcode)->hasTranslationChanges()
reportsTRUE
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.
Comment #48
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedRemoved
Paragraph
class leftover from #47Comment #49
aschiwi CreditAttribution: aschiwi at undpaul commented@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.
Comment #50
recrit CreditAttribution: recrit at Phase2 commented3025709-48.patch worked for me.
New node revisions were created when only a paragraph field was changed.
Comment #51
BerdirAfter 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.
Comment #53
maximpodorov CreditAttribution: maximpodorov commentedSo, new release is coming, right?
Comment #55
arsn CreditAttribution: arsn commentedYes. It has been fixed in version 1.7 on the 5th of October, 2019