Problem/Motivation
See #2807371: META Support Content Moderation module and #2951436: Fix integration with content moderation in multi-lingual scenarios as well as the core issue #2960253: [meta] Allow Paragraphs widget/field and similar use cases to to be considered translatable
This issue is about the storage part needed for this.
There are several aspects that need to be resolved:
1. Prevent untranslatable fields from being copied and overwriting translation revisions
2. Instead, pass the createRevision() call recursively down to referenced composite entities
3. Correctly report/handle affected languages, so that the UI can correctly show which revisions changed in a specific language.
.. more?
Proposed resolution
This uses the experimental hook from the core issue to do first two things for now.
Comments
Comment #2
BerdirComment #4
BerdirSupporting the new FieldItemList interface to control the really affected languages.
Comment #6
plachforwarding the
keep_untranslatable_fields
flag makes sense only if the target entity is configured in the same way as the referencing entity. Can we rely on that or should we recompute the flag value?We decided to adopt the "pending revision" terminology instead of "forward revision", since a pending revision may precede a default revision when dealing with translated entities.
Comment #7
Berdir1. Yeah, I'm not sure about those arguments either. We don't know if the "incoming" call relied on the default fallback and was NULL or explicitly set. We could call the hook before setting it, so we pass on NULL if it wasn't set? If we do need to check something on that setting, then it would be tricky though. It's worth pointing out that I think the paragraph widget will need to automatically apply the parent setting there anyway, especially when using content moderation, because it will not be automatically forced, but we expect it to be consistent anyway.
I'd also specifically love to get your feedback on this.
The second two calls are likely not needed, was just experimenting when things didn't work as expected.
However, the first two are, without having content_translation enabled and having the bundles enabled for translation, nothing works, because the createRevision() call checks $entity->isTranslatable() which looks at the translatable key of the bundle definition.
This seems strange to me, in my mind, content_translation is just the UI, the storage/entity type *is* translatable without it, I can save translations just fine.
Don't know if we can/should do something about that (e.g. default to translatable if the storage allows it if the translable key is not set at all?), it was certainly pretty confusing and took me a while to track down while writing the test.
Comment #8
BerdirImproved test coverage for the default revision, forward => pending.
Comment #10
BerdirThis is re-rolled on top of #2953343: Experimental asymmetrical content translation with Paragraphs breaks concurrent drafts, you will need that patch even when not using translatable fields as that now contains the important changes on default/new revision handling that are also needed for this.
Comment #12
BerdirExperimenting with drupalci.yml file to be able to apply the required core patch.
Comment #14
BerdirOops.
Comment #16
BerdirComment #18
BerdirUsing the new method from the core issue, updating the used patch.
Comment #20
BerdirAlso using the correct patch name here.
Comment #22
BerdirOk, those test fails are valid and caused by the fact that we didn't check for actual changes like more/fewer and different entity references. Improved that and added more explicit test coverage for it as well.
Comment #23
BerdirUpdating for the most recent changes in the core issue.
Comment #25
BerdirAnother patch fail, should not post patches on friday after 7pm :)
Comment #26
BerdirUpdated patch for the latest changes in the core issue.
Comment #27
BerdirExpanding test coverage, adding an initial published de translation to see the differences between having and not having that as well as adding an untranslatable text field on the composite, this was failing on the paragraph tests.
Comment #28
JeroenTComment #29
miro_dietikerI just released ERR 8.x-1.5 with a limitation note about this issue.
Needs a reroll to test, maybe postpone then on core. Hope core catches up with this subject soon so we can make our users happy.
Comment #30
BerdirNew patch, no longer combined with the other one as that was committed.
Comment #32
MixologicI mentioned this in slack, but for everybody elses sake, we've rearranged everything so that its all owned by the www-user inside of the container. Try changing the command that applies the additional core patch as:
sudo -u www-user curl "https://www.drupal.org/files/issues/2018-05-07/content-translation-untranslatable-paragraphs-2960253-20.patch" | sudo -u www-user patch -p1
Comment #33
BerdirThanks, lets try that, also updated to use the latest patches from the new child issues.
Comment #35
BerdirMore sudo.
Comment #37
BerdirCopy & Pasted the wrong username :)
Comment #38
eyilmazHi!,
here is a bug introduced with the latest patch.
Whats needed:
- a node with content moderation and a paragraphs field (field_paragraph).
- 2 paragraphs, 1 wrapper & 1 normal paragraph, where wrapper can be added to the paragraphs field (field_paragraph) in the node.
- wrapper has another err field where the "normal paragraph" can be added.
- The form-display for the field field_paragraph should be set to preview.
--> so we have a clear usecase of paragraphs using paragraphs.
Steps to reproduce:
- Create a node, and add to field_paragraph 2 paragraphs of type wrapper with normal paragraphs in it.
- Publish the node.
- Create a new draft of that node.
- Click edit for the first wrapper paragraph in the field_paragraph field. --> Expected and real behaviour, you are able to edit the paragraph.
- Click edit for the second wrapper paragraph in the field_paragraph field --> Expected behaviour: I can edit the paragraph, Current behaviour: The previously created paragraph is not there anymore.
I applied all patches. as described in the docu page from https://www.drupal.org/docs/8/modules/paragraphs/multilingual-and-conten.... And the described bug went away when I removed the patch from this issue, so assuming that here must be something wrong.
Comment #39
eyilmazHere is a short gif showing the issue.
Comment #40
BerdirThanks for the detailed report. Is that a multilingual site, are translations enabled for those paragraphs, also for the wrapper paragraph type?
I'll look into reproducing it.
Comment #41
eyilmazthe origin site where the bug appeared was multilingual..
But I did a fresh install with just paragraphs, no multilingual. The bug seems to appear only when content moderation is enabled.
I could see that for the second call, the form is retrieved from the cache and the hooks like content_moderation_entity_prepare_form (and depending from that entity_reference_revisions_entity_revision_create) are not called.
Comment #42
cosmicdreams CreditAttribution: cosmicdreams commentedI've run into issues with this patch as well.
It seems as if the hasAffectingChanges is impacting situations where second level paragraphs are used. Therefore I can't have a structure like:
Content Type: Page
has a
Paragraph Type: Layer
Which can have other paragraph types.
Any change to the default language version of the Page trips the constraint. Any change to the non default language version of a Page trips the constraint. I'm investigating what is possible but would really appreciate to collaborate on the fix as time is very short for me.
This is soon to impact a whole suite of sites I help manage. Quite a surprise.
Comment #43
cosmicdreams CreditAttribution: cosmicdreams commentedAs I try to walk through the issue conceptually in order to write a comment here that explains my confusion, a logic break hit me. I'm trying to reason through why this has become a problem now and wasn't a problem before we migrated to content_moderation from workbench_moderation. Walk through this reasoning here and help me see what I've got wrong:
EntityUntransableFieldsConstraintValidator is the object that is responsible for adding the form validation violation that prevents my node from saving. It's boolean method: hasUntranslatableFieldsChanges is intended to check all the fields involved in the form submission and report back if any untranslateable fields have a change.
In this case, the fields that are untranslateable are the fields that record which paragraphs I'm referencing in whether I've added a new 2nd level paragraph.
Luckily, this module overrode the hasAffectingChanges() method that ultimately determines whether to report back if the change to these untranslateable fields is supposed to be an affecting change.
Is the goal of this "fix" to figure out a way to continue to report back if the affecting change will cause nodes to forget the paragraphs they should be managing, while at the same allow new paragraphs to be added to a node without compliant?
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedFound a test case that demonstrates my issue. Here are the steps:
Prerequisites:
Have content_moderation, content_translation, and paragraphs enabled.
Have at least 2 languages
1. Create a node that has paragraphs, while creating that node the first time, add a paragraph item. Save the node.
2. Edit that same node, modify a field on the paragraph item that isn't an entity_reference. Like a text field. Try to save the node.
3. Fail
This fails because hasAffectingChanges() considers this to be a Translation change, even though you are attempting to modify the node in it's default language.
This is a critical error
Comment #45
cosmicdreams CreditAttribution: cosmicdreams commentedComment #46
cosmicdreams CreditAttribution: cosmicdreams commentedAlso fails when you try to change the order of a paragraph's entity references. In the case where you have a paragraph that's something like a grid, where it's job is to reference other content.
Comment #47
cosmicdreams CreditAttribution: cosmicdreams commentedI've found a solution that works for me. But I think I'm going to need someone explain to me why this is a bad idea because this seems too heavy-handed to me:
In EntityReferenceRevisionsFieldItemList
Comment #48
BerdirWell, return FALSE is not correct because we must report changes if there really are any, like new paragraphs/order changes in when editing in the default language, otherwise no languages would be marked as affected and the revision would for example not show up.
What would help is if you could extend my tests, either in this issue or in the paragraphs issue. But as mentioned there, if you are using translatable fields then the situation is different anyway.
Comment #49
cosmicdreams CreditAttribution: cosmicdreams commentedHi @Berdir!
Thanks for responding.
In my testing, I haven't seen the "revision not showing up" issue you're talking about. But it would be awesome if I could add a test.
What's concerning is that with the hasAffectingChanges() as it currently written, I can not make changes to the default language version of the content. I'm trying to do common use case things, like change which entities are references inside a second level paragraph. Or Modifying text fields within a second level paragraph. Things my content authors expect to do.
What doesn't make sense to me is why the Constraint and hasAffectingChanges prevents me from doing whatever I want to the content while editing it in it's default language. I understand why you would want to report to the other languages that changes are being made that would mark them as out of sync. But I don't understand why that would trigger the constraint and add a violation.
Perhaps the dual purpose of hasAffectingChanges() needs to be separated. Have one method that decides whether the constraint should be triggered and another method decide when to report translation changes to the other translations.
Comment #50
vincejones CreditAttribution: vincejones commentedWorking with @cosmicdreams the immediate fix to our problem was because we did not checking the "Hide non translatable fields on translation forms" checkbox for every paragraph type on content language settings page, this should be checked. More testing to come to ensure this is fixed for our needs. I will report back if further fixes are needed.
Comment #51
BerdirRemoved the drupalci.yml file now that all core patches have been committed.
Comment #52
mpp CreditAttribution: mpp at AmeXio for District09 commentedThanks for the patch.
As FieldDefinitionInterface::getType returns a string we should probably use === instead of ==
Comment #53
mediabounds CreditAttribution: mediabounds commentedI'm also experiencing the issue that @eyilmax described in #38 where I have a host node with content moderation and a nested paragraph that disappears during editing of the node. I was able to fix it by augmenting the patch in #51 with this patch:
I'm not attaching this as a patch because I only tested it with content moderation enabled so I don't know how it impacts sites not using that module. And candidly, I don't fully understand why this works. I am simply mimicking what is done in Content Moderation in EntityTypeInfo::entityPrepareForm.
It looks like a few more people are experiencing this issue and are reporting it to the Paragraphs project:
https://www.drupal.org/project/paragraphs/issues/2951436#comment-12657283
https://www.drupal.org/project/paragraphs/issues/2951436#comment-12740967
https://www.drupal.org/project/paragraphs/issues/2951436#comment-12766351
https://www.drupal.org/project/paragraphs/issues/2994937
The key in all instances seems to be having content moderation enabled on the host node type.
Comment #54
Berdir@mediabounds, thanks that is helpful.
Could you maybe expand the test coverage to trigger this bug? I'm not quite sure why it doesn't happen with the test coverage we have...
Comment #55
mpp CreditAttribution: mpp at AmeXio for District09 commentedWe'd probably need a test to check that a text paragraph A still exists when making a modification on a nested paragraph B.
Comment #56
merauluka CreditAttribution: merauluka at Mediacurrent commentedI'm attaching an updated patch with #53 incorporated. This appears to resolve the errors that I've been getting locally on save where:
The combined patches from #51 and #53 appear to resolve both issues listed above. This is one a client site with:
Comment #57
Anybody#56 works for us on 8.6.1 and fixed the problem, thanks!
Comment #58
petermallett CreditAttribution: petermallett at Mediacurrent commentedThe patch in #56 appears to work, however after making an edit to a paragraph in a node translation & then returning to the default language edit form & attempting an edit, the save fails with the message: "The content has either been modified by another user, or you have already submitted the modifications. As a result, your changes cannot be saved".
Comment #59
johnchqueHmm I cannot reproduce the error described in #58. Anyway I created some tests for that too, seems I cannot reproduce the error either.
@petermallett can you describe your translation settings of the paragraph field and paragraphs?
Comment #61
Berdirwe don't do paragraphs ui tests in ERR, as you can see we also don't have that dependency yet.
If it is a storage problem then it should be reproducible in the existing kernel test.
Comment #62
johnchqueTried with a different instance of Drupal with all translations settings set according to this. And I am not able to reproduce the bug described in #58
@petermallett please provide clear steps to reproduce the problem.
Re uploading the last patch from #56. At least until we have clear descriptions of other problems.
Comment #63
BerdirOk, I can now manually reproduce the problem when enabling content moderation in paragraphs_demo (will upload a patch in the paragraphs issue that does that by default) and then do the following steps on the example node there and then add a new paragraph and either save or even just open the nested paragraph.
> And candidly, I don't fully understand why this works. I am simply mimicking what is done in Content Moderation in EntityTypeInfo::entityPrepareForm.
It also took me a while and it took me a very long time to figure out a way to reproduce it in a kernel test.
I think the problem was that having them closed didn't trigger the needs save flag, and then they don't have a revision ID, which results in \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::preSave() getting NULL and saving NULL in the target_revision_id.
I added nested composite and tried to create revisions but nothing broke until I found it. The fun part is, the very same code that fixes this also breaks it, because when the revision ID is set again, then isNewRevision() returns FALSE, so in that preSave(), first we don't save a new revision and then later, we expect one to be there.
Maybe we need to handle those cases in preSave() better, I imagine this might result in currently not actually creating a new nested revision in some cases.
Comment #64
BerdirObviously those were the wrong files.
Comment #67
BerdirDidn't know that the name has a length limit, and it didn't fail on Sqlite.
Comment #68
petermallett CreditAttribution: petermallett at Mediacurrent commented@Birdir & @yongt9412, thanks so much for diving into this, sorry I was out of town since last Thursday & wasn't able to provide more details.
On initial testing, it appears the patch in #67 does correct the issue we were seeing.
In case it is still helpful for later (in case there's any other work to do / issues to uncover) here is the setup we have:
If you test this scenario w/ #56, you should be able to recreate this issue:
* Drupal core version 8.5.6
* all patches from this page applied: https://www.drupal.org/docs/8/modules/paragraphs/multilingual-and-conten...
* content moderation & translation in play on the node in question
* a translatable paragraph within a paragraph (ERR fields not set to translatable, all in accordance with https://www.drupal.org/node/2735121)
* Test node is in language A, also translated into language B.
** Edit a paragraph in language B (translated) version, save (draft or published, either works).
** navigate to the original language version of the node edit form
** attempt a paragraph edit in orig. lang. version & the error occurred
It appears that #67 does fully correct this, however.
edit: I must have made a mistake while testing: #67 does not fully correct this :(
Comment #69
BerdirWorking on supporting to sync with the paragraphs from the latest revision in the default language. This will also need storage tests in ERR.
Comment #70
petermallett CreditAttribution: petermallett at Mediacurrent commentedDid some testing with #69 this morning: Finding are that for _some_ flows, this is working (like if you edit & save straight to "Published", skipping "Draft").
Here's the workflow I used to cause the "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved." message:
I've attached a video capture of that process from my local setup as well.
Comment #71
johnchqueAs written in #2951436: Fix integration with content moderation in multi-lingual scenarios:
While working on this, I did find a problem, when:
- Create a published node in EN with a Text Paragraph and a Nested one that contains another Text Paragraph.
- Create a draft, remove the first Text Paragraph, add a second text Paragraph inside the Nested one.
- Translate the node, with the current implementation, we should get the structure of the published EN node.
- The deleted paragraph in draft is still present in the translation form as expected, however, the Paragraph added inside the Nested one, is also displayed when it shouldn't.
It seems that we are not passing the right translation of the Nested Paragraph yet. Because in ERR when checking if !$new_revision->isDefaultTranslation() for node it returns TRUE but for the Nested Paragraph it returns FALSE, in entity_reference_revisions_entity_revision_create().
Comment #72
BerdirExtended test coverage to test for the behavior that we are now implementing as a first version.
Made two changes to make those tests work, first, before, we only used a translation of a paragraph if it existed. Now we need to create it if it doesn't. Second, we need to always ensure the paragraph structure, not just on pending revisions if it's a translation, for the case when a translation is published on top of a new default revision, then it is becoming the default revision but can still be out of sync.
@petermallett: This approach should prevent validation errors, but as mentioned, this *will* change the process and it will not be possible to prepare draft translations of a new draft before that is published. This is the only feasible first step because that is how core expects untranslatable fields to behave and changing it will require changes to how it is validated and revisions are created in core.
Comment #74
BerdirAgain that *** length limit :)
Comment #75
petermallett CreditAttribution: petermallett at Mediacurrent commented@berdir Sorry, I'm not quite following your last comment on how this will work with drafts + translations -- were you saying that for now we will need to enforce updating/publishing the orig.-language version before publishing translated drafts? (that workflow does work w/ all the latest described below. ie: have 2 langs, make draft in translation, make update to orig. + move to published. *then* publish translation draft -- everything works.).
Also, the scenario we have is with a node that has existing, published versions in both the orig. & a translated language, not newly created. I'm not sure if that was clear and/or matters :)
Also just for complete info, Im now testing with
- drupal/core:8.5.6
- drupal/paragraphs:dev-1.x#2cb0efcba81a0eb2707ec443f954c586e3112fd5
- drupal/entity_reference_revisions:dev-1.x#e78d3d3256b975cef7d48cdec68bc83759880dab
- Content Moderation enabled
- Multi-lingual site, 5 languages enabled for content translation
- everything from this page https://www.drupal.org/docs/8/modules/paragraphs/multilingual-and-conten... (Multilingual and Content Moderation)
- As well as this patch since it is not clear if it is still required or not: https://www.drupal.org/project/paragraphs/issues/2991757
Comment #76
BerdirLets say you have an existing node in EN and a DE translation. When you add a draft in EN and for example add a new paragraph and then update the translation of DE, you will *not* see that new paragraph until the EN draft has been published.
See the paragraphs issue, documentation page and the related follow-up that we created. We can not change that without changes/patches in Drupal core and in turn, we couldn't commit something to ERR/Paragraphs that relies on core patches to work, so we decided to commit this as phase 1, do new releases and then look into supporting that.
Comment #77
wouter.adem CreditAttribution: wouter.adem commentedThis is just a remark on the code in the patch #70 where I think putting the
if
statement on line 144 before theforeach
on line 131 would be better. This is in the methodhasAffectingChanges
.Comment #78
maximpodorov CreditAttribution: maximpodorov commentedApplying the optimization proposed in #77.
Comment #80
miro_dietikerAwesome, let's move forward. :-)
Comment #81
maximpodorov CreditAttribution: maximpodorov commentedIt's not me who should be credited here.
Comment #82
miro_dietikerYeah sorry, it automatically picks the last and i forgot to switch the radio button... Can't change anymore. Also it's still really hard to handle attribution appropriately in large collaborative issues, sorry also for everyone i accidentally skipped.
Comment #83
attisanI'm not yet 100% sure - but it seems to me, this patch creates an "Invalid translation language (zxx) specified." InvalidArgumentException by trying to add translations to entities defined as untranslatable (e.g. "untranslatable" zxx nodes with paragraph fields).
Though the node storage is instanceof TranslatableRevisionableStorageInterface, the node bundle itself is set untranslatable and has zxx set as default langcode.
perhaps an extra check, regarding the entity / bundle "translatability" is needed in such case?
Comment #84
Berdir@attisan: Thanks for reporting, can you create a new issue and provide steps to reproduce? And then reference it here.
Comment #86
alberto56 CreditAttribution: alberto56 at Dcycle commentedGetting the same problem as @attisan in #83, following up in #3024588: InvalidArgumentException: Invalid translation language (und) specified. in Drupal\Core\Entity\ContentEntityBase->addTranslation() (line 953 of core/lib/Drupal/Core/Entity/ContentEntityBase.php).
Comment #87
zaporylieI have created one more regression report #3025709: "Create new revision" option is ignored when updating EntityReferenceRevisionsItem