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.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#78 err-forward-translation-revision-2961399-78-interdiff.txt912 bytesmaximpodorov
#78 err-forward-translation-revision-2961399-78.patch33.36 KBmaximpodorov
#74 err-forward-translation-revision-2961399-74-interdiff.txt734 bytesBerdir
#74 err-forward-translation-revision-2961399-74.patch33.36 KBBerdir
#72 err-forward-translation-revision-2961399-72-interdiff.txt18.32 KBBerdir
#72 err-forward-translation-revision-2961399-72.patch33.37 KBBerdir
#71 interdiff-2961399-69-70.txt1.14 KBjohnchque
#71 err-forward-translation-revision-2961399-70.patch28.97 KBjohnchque
#69 err-forward-translation-revision-2961399-69-interdiff.txt2.66 KBBerdir
#69 err-forward-translation-revision-2961399-69.patch29.15 KBBerdir
#67 err-forward-translation-revision-2961399-67-interdiff.txt9.5 KBBerdir
#67 err-forward-translation-revision-2961399-67.patch28.03 KBBerdir
#64 err-forward-translation-revision-2961399-63-interdiff.txt14.51 KBBerdir
#64 err-forward-translation-revision-2961399-63.patch28.15 KBBerdir
#63 comment-fix-the-DX-problems-2975217-27-interdiff.txt1.05 KBBerdir
#63 comment-fix-the-DX-problems-2975217-27.patch5.72 KBBerdir
#62 err-forward-translation-revision-2961399-62.patch24.89 KBjohnchque
#59 interdiff-2961399-56-59.txt5.56 KBjohnchque
#59 err-forward-translation-revision-2961399-59.patch30.15 KBjohnchque
#56 err-forward-translation-revision-2961399-56-interdiff.txt1.14 KBmerauluka
#56 err-forward-translation-revision-2961399-56.patch24.89 KBmerauluka
#51 err-forward-translation-revision-2961399-51.patch24.63 KBBerdir
#39 2961399_bug.gif3.43 MBeyilmaz
#37 err-forward-translation-revision-2961399-37-interdiff.txt830 bytesBerdir
#37 err-forward-translation-revision-2961399-37.patch26.22 KBBerdir
#35 err-forward-translation-revision-2961399-35-interdiff.txt689 bytesBerdir
#35 err-forward-translation-revision-2961399-35.patch26.22 KBBerdir
#33 err-forward-translation-revision-2961399-33.patch26.19 KBBerdir
#30 err-forward-translation-revision-2961399-29.patch26.08 KBBerdir
#27 err-forward-translation-revision-2961399-27-combined-interdiff.txt14.47 KBBerdir
#27 err-forward-translation-revision-2961399-27-combined.patch48.63 KBBerdir
#26 err-forward-translation-revision-2961399-26-combined-interdiff.txt2.17 KBBerdir
#26 err-forward-translation-revision-2961399-26-combined.patch45.13 KBBerdir
#25 err-forward-translation-revision-2961399-23-combined-interdiff.txt4.21 KBBerdir
#25 err-forward-translation-revision-2961399-23-combined.patch45.11 KBBerdir
#23 content-translation-untranslatable-paragraphs-2960253-15-interdiff.txt8.61 KBBerdir
#23 content-translation-untranslatable-paragraphs-2960253-15.patch9.05 KBBerdir
#22 err-forward-translation-revision-2961399-22-combined-interdiff.txt3.12 KBBerdir
#22 err-forward-translation-revision-2961399-22-combined.patch44.91 KBBerdir
#20 err-forward-translation-revision-2961399-20-combined-interdiff.txt548 bytesBerdir
#20 err-forward-translation-revision-2961399-20-combined.patch42.79 KBBerdir
#18 err-forward-translation-revision-2961399-18-combined-interdiff.txt2.01 KBBerdir
#18 err-forward-translation-revision-2961399-18-combined.patch42.79 KBBerdir
#16 err-forward-translation-revision-2961399-16-combined-interdiff.txt631 bytesBerdir
#16 err-forward-translation-revision-2961399-16-combined.patch43.18 KBBerdir
#14 err-forward-translation-revision-2961399-13-combined-interdiff.txt653 bytesBerdir
#14 err-forward-translation-revision-2961399-13-combined.patch43.16 KBBerdir
#12 err-forward-translation-revision-2961399-12-do-not-test.patch20.61 KBBerdir
#12 err-forward-translation-revision-2961399-12-combined.patch42.86 KBBerdir
#10 err-forward-translation-revision-2961399-10.patch19.18 KBBerdir
#8 err-forward-translation-revision-2961399-8-interdiff.txt12.48 KBBerdir
#8 err-forward-translation-revision-2961399-8.patch20.05 KBBerdir
#4 err-forward-translation-revision-2961399-4.patch19.68 KBBerdir
#2 err-forward-translation-revision-2961399-2.patch17.23 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
17.23 KB

Status: Needs review » Needs work

The last submitted patch, 2: err-forward-translation-revision-2961399-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs work » Needs review
FileSize
19.68 KB

Supporting the new FieldItemList interface to control the really affected languages.

Status: Needs review » Needs work

The last submitted patch, 4: err-forward-translation-revision-2961399-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

  1. +++ b/entity_reference_revisions.module
    @@ -218,3 +220,29 @@ function entity_reference_revisions_form_field_ui_field_storage_add_form_alter(a
    +            $item->entity = $target_storage->createRevision($target_entity, $context['default'], $context['keep_untranslatable_fields']);
    

    forwarding 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?

  2. +++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTranslationTest.php
    @@ -0,0 +1,281 @@
    +    // Now publish the FR forward revision.
    

    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.

Berdir’s picture

1. 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.

+++ b/tests/src/Kernel/EntityReferenceRevisionsCompositeTranslationTest.php
@@ -0,0 +1,281 @@
+    // @todo content_translation should not be needed for a storage test, but
+    //   \Drupal\Core\Entity\ContentEntityBase::isTranslatable() only returns
+    //   TRUE if the bundle is explicitly translatable.
+    \Drupal::service('content_translation.manager')->setEnabled('node', 'article', TRUE);
+    \Drupal::service('content_translation.manager')->setEnabled('entity_test_composite', 'entity_test_composite', TRUE);
+    \Drupal::service('content_translation.manager')->setBundleTranslationSettings('node', 'article', [
+      'untranslatable_fields_hide' => TRUE,
+    ]);
+    \Drupal::service('entity_type.bundle.info')->clearCachedBundles();

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.

Berdir’s picture

Improved test coverage for the default revision, forward => pending.

Status: Needs review » Needs work

The last submitted patch, 8: err-forward-translation-revision-2961399-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs work » Needs review
FileSize
19.18 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 10: err-forward-translation-revision-2961399-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Experimenting with drupalci.yml file to be able to apply the required core patch.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 14: err-forward-translation-revision-2961399-13-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 16: err-forward-translation-revision-2961399-16-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Using the new method from the core issue, updating the used patch.

Status: Needs review » Needs work

The last submitted patch, 18: err-forward-translation-revision-2961399-18-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 20: err-forward-translation-revision-2961399-20-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Ok, 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.

Berdir’s picture

Status: Needs review » Needs work
Berdir’s picture

Berdir’s picture

Berdir’s picture

Expanding 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.

JeroenT’s picture

Title: Support parallel translation forward revisions on untransltable fields » Support parallel translation forward revisions on untranslatable fields
miro_dietiker’s picture

Status: Needs review » Needs work

I 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.08 KB

New patch, no longer combined with the other one as that was committed.

Status: Needs review » Needs work

The last submitted patch, 30: err-forward-translation-revision-2961399-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mixologic’s picture

I 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

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.19 KB

Thanks, lets try that, also updated to use the latest patches from the new child issues.

Status: Needs review » Needs work

The last submitted patch, 33: err-forward-translation-revision-2961399-33.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 35: err-forward-translation-revision-2961399-35.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Copy & Pasted the wrong username :)

eyilmaz’s picture

Status: Needs review » Needs work

Hi!,

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.

eyilmaz’s picture

FileSize
3.43 MB

Here is a short gif showing the issue.

Berdir’s picture

Thanks 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.

eyilmaz’s picture

the 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.

cosmicdreams’s picture

Issue summary: View changes

I'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.

cosmicdreams’s picture

As 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?

cosmicdreams’s picture

Found 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

cosmicdreams’s picture

cosmicdreams’s picture

Also 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.

cosmicdreams’s picture

I'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

 /**
   * {@inheritdoc}
   */
  public function hasAffectingChanges(FieldItemListInterface $original_items, $langcode) {
    return FALSE;
  }
Berdir’s picture

Well, 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.

cosmicdreams’s picture

Hi @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.

vincejones’s picture

Working 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
24.63 KB

Removed the drupalci.yml file now that all core patches have been committed.

mpp’s picture

Thanks for the patch.

+    if ($field_definition->getType() == 'entity_reference_revisions' && !$field_definition->isTranslatable()) {

As FieldDefinitionInterface::getType returns a string we should probably use === instead of ==

mediabounds’s picture

I'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:

diff -u b/entity_reference_revisions.module b/entity_reference_revisions.module
--- b/entity_reference_revisions.module
+++ b/entity_reference_revisions.module
@@ -241,7 +241,12 @@
           $values = [];
           foreach ($entity->get($field_name) as $delta => $item) {
             $target_entity = $item->entity->hasTranslation($active_langcode) ? $item->entity->getTranslation($active_langcode) : $item->entity;
-            $values[$delta] = $target_storage->createRevision($target_entity, $new_revision->isDefaultRevision(), $keep_untranslatable_fields);
+            $revised_entity = $target_storage->createRevision($target_entity, $new_revision->isDefaultRevision(), $keep_untranslatable_fields);
+
+            // Restore the revision ID.
+            $revision_key = $revised_entity->getEntityType()->getKey('revision');
+            $revised_entity->set($revision_key, $revised_entity->getLoadedRevisionId());
+            $values[$delta] = $revised_entity;
           }
           $new_revision->set($field_name, $values);
         }

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.

Berdir’s picture

@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...

mpp’s picture

We'd probably need a test to check that a text paragraph A still exists when making a modification on a nested paragraph B.

merauluka’s picture

I'm attaching an updated patch with #53 incorporated. This appears to resolve the errors that I've been getting locally on save where:

  • The sample text provided for a "closed" translated paragraph is displayed in the original language rather the translated language.
  • Once the paragraph is opened to edit, the revision appears in the correct language, however any updates since the last revision weren't shown. From what I can tell, it appeared that the system was loading the first revision for that translation.

The combined patches from #51 and #53 appear to resolve both issues listed above. This is one a client site with:

  • Core 8.5.6
  • Core's content moderation
  • Translated, nested paragraphs with paragraphs on the latest dev
Anybody’s picture

#56 works for us on 8.6.1 and fixed the problem, thanks!

petermallett’s picture

The 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".

johnchque’s picture

Hmm 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?

Status: Needs review » Needs work

The last submitted patch, 59: err-forward-translation-revision-2961399-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

+++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
@@ -0,0 +1,135 @@
+ *
+ * @group entity_reference_revisions
+ */
+class EntityReferenceRevisionsCompositeTest extends WebTestBase {
+
+  use FieldUiTestTrait;
+  use EntityReferenceRevisionsCoreVersionUiTestTrait;
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array(
+    'node',
+    'field',
+    'entity_reference_revisions',
+    'field',
+    'field_ui',
+    'block',

we 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.

johnchque’s picture

Status: Needs work » Needs review
FileSize
24.89 KB

Tried 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.

Berdir’s picture

Ok, 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.

Berdir’s picture

The last submitted patch, 63: comment-fix-the-DX-problems-2975217-27.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 64: err-forward-translation-revision-2961399-63.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Didn't know that the name has a length limit, and it didn't fail on Sqlite.

petermallett’s picture

@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 :(

Berdir’s picture

Working on supporting to sync with the paragraphs from the latest revision in the default language. This will also need storage tests in ERR.

petermallett’s picture

Did 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:

  1. start with a node with a translation
  2. edit paragraph in translation, save as draft
  3. note the moderation state of the orig. language is affected
  4. save the translation draft from step 2 as published
  5. note the moderation state of the orig. language is still draft in the Translation tab, but not on its edit form
  6. attempt to edit a paragraph in the orig. language, result is the error

I've attached a video capture of that process from my local setup as well.

johnchque’s picture

As 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().

Berdir’s picture

Extended 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.

Status: Needs review » Needs work

The last submitted patch, 72: err-forward-translation-revision-2961399-72.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Again that *** length limit :)

petermallett’s picture

@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

Berdir’s picture

Lets 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.

wouter.adem’s picture

This is just a remark on the code in the patch #70 where I think putting the if statement on line 144 before the foreach on line 131 would be better. This is in the method hasAffectingChanges.

maximpodorov’s picture

miro_dietiker’s picture

Status: Needs review » Fixed

Awesome, let's move forward. :-)

maximpodorov’s picture

It's not me who should be credited here.

miro_dietiker’s picture

Yeah 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.

attisan’s picture

Status: Fixed » Needs work

I'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.

33
InvalidArgumentException 
…/web/core/lib/Drupal/Core/Entity/ContentEntityBase.php : 953
32
Drupal\Core\Entity\ContentEntityBase addTranslation
…/web/modules/contrib/entity_reference_revisions/entity_reference_revisions.module : 267
31
 entity_reference_revisions_entity_revision_create
…/web/core/lib/Drupal/Core/Extension/ModuleHandler.php : 403
30
 call_user_func_array
…/web/core/lib/Drupal/Core/Extension/ModuleHandler.php : 403

... 

perhaps an extra check, regarding the entity / bundle "translatability" is needed in such case?

Berdir’s picture

Status: Needs work » Fixed

@attisan: Thanks for reporting, can you create a new issue and provide steps to reproduce? And then reference it here.

Status: Fixed » Closed (fixed)

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

zaporylie’s picture