Problem/Motivation

When importing a translation with Auto accept finished translations enabled on the translation provider, the new translated value doesn't get added to any paragraphs.

If you disable the Auto accept finished translation option and accept the translation from the Job items review form, it all works as excepted.

Looking at one of the paragraphs fields table you can see that a new revision is created with the new translation. But then another revision is created with the old value, and that's the paragraph that gets referenced on its host.

Using 8.x-1.11 it works as expected.

I did some digging and it looks like this bug was introduced in #3162640: Support translation of translatable fields with untranslatable target types. Reverting the commit on 8.x-1.12 fixes the issue.

Steps to reproduce

  1. Install site with standard profile
  2. Install paragraphs module
  3. Install tmgmt, tmgmt_content, tmgmt_file
  4. Enable Auto import in the File exchange provider
  5. Create a new paragraphs type (e.g. Text)
  6. Add a text field to the paragraph type
  7. Add a paragraphs field to the the basic_page content type that can reference the Text paragraph
  8. Configure a second language to the site (Swedish)
  9. Configure the basic_page and paragraph to be translatable (paragraph field on the basic_page is left untranslatable)
  10. Create a new node of type basic_page, add a Text paragraph to it and publish it
  11. Create and submit a new translation job with the File exchange provider and translate the basic_page to Swedish
  12. Download the XLIFF export and update the target fields with something to differentiate the translation (e.g. "Translation 1")
  13. Upload the "translated" XLIFF file in on the Jobs view and import it
  14. Check the translated page - the Swedish translation should be imported as expected
  15. Do step 10-12 again but update the XLIFF export with something new (e.g. "Translation 2").
  16. Inspect the Swedish translation again.

You expect the new translation ("Translation 2") to be available on the paragraph. Instead the initial translation ("Translation 1") is there.

Proposed resolution

Find what causes the issue and fix it. Do we add a new test for this? I'll continue investigating today and next week and try to do a merge request if I find something.

CommentFileSizeAuthor
#16 Screenshot 2021-05-03 at 11.38.12.png487.43 KBitamair

Issue fork tmgmt-3207993

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcusml created an issue. See original summary.

marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented
Related issues: +#3162640: Support translation of translatable fields with untranslatable target types
marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented
Title: Paragraphs translations aren't updated when imported with Auto accept finished translations enabled » Paragraphs translations aren't updated when imported with file import (with Auto accept finished translations enabled)

I've tested some more and the translation can be imported and update the paragraphs correctly with Auto accept enabled. It works if you save the translation using the job item form instead of uploading the translation file. If you don't have Auto accept enabled you are forced to save the translation in the job item form, that's why this issue is only apparent with auto accept enabled.

Although #3207997: Paragraphs translations are not added while save as completed seems to describe the same issue but when saving the translations using the form. But I haven't been able to reproduce that myself.

There are some differences to the translation data that gets saved to the job item and later passed to the ContentEntitySource. The form adds a '#published' item on the data array and the translations array has '#origin' set to 'local' compared to 'remote' when importing the file. 

               '#status' => 2,
               '#translation' =>
                 array (
-                  '#text' => 'Translations 2',
-                  '#origin' => 'remote',
-                  '#timestamp' => 1618137033,
+                  '#text' => 'Translation 3',
+                  '#origin' => 'local',
+                  '#timestamp' => 1618137685,
                 ),
             ),
         ),

But I tried forcing the file import to produce an identical data array as when saving with the form, and it still wouldn't work.

marcusml opened merge request !3

marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented

Just wanted to share my latest finding. I found that the entity passed to ContentEntitySource::doSaveTranslations is different depending on if you import the translation on the job form than if you use the job item form, I guess because of different operations that's been done on the entity during the request. The only way I'm able to reproduce the issue outside of \tmgmt_file_import_form_submit is by clearing the entity cache before loading the entity that we're passing to ContentEntitySource::doSaveTranslations. As is done in the current failing test in the MR.

I'm not knowledgable enough about the Entity API to understand why it behaves is it does. But I'll continue digging and hopefully I can make some progress tomorrow.

marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented
Assigned: marcusml » Unassigned
Status: Active » Needs review

I just pushed a new commit which fixes the issue. I'm not very confident in the change though. I guess the field was cloned for a particular reason, but the change doesn't seem to fail any existing tests.

Berdir made their first commit to this issue’s fork.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

> I just pushed a new commit which fixes the issue. I'm not very confident in the change though.

You should be, that's some amazing work tracking this one down. Took me quite some time to understand this one myself and I'm not sure I really _exactly_ get it.

I can't think of a reason why that clone was added, I only see in the original issue that it was added at the same time as the one below, so maybe @mbovan thought it would be more consistent? But it doesn't have to be as it works differently.

What's roughly happening is that we rely on ERR's autosave and revision update capabilities, but only implicitly, which updates the target revision id in \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::preSave. But by cloning it, we break the object reference to that entity object, so in there, we do see that the parent is saved as a new reference, but the entity object we have there is unchanged, so it has the old values and revision id. That is combined with the fact that, because we also see that the parent is saved as a new revision, we do the same to the referenced entities, resulting in a second new revision with the old values and then we store the reference to that.

The reason it's OK with the new untranslatable reference support is that there we explicitly set the value of the new entity on the translation field object. We could do the same, and set the value again on the $translation field object, that would fix the error too.

I also looked into the duplicated revision thing, and I think we can prevent that relatively easily by relying on the EntityNeedsSaveInterface provided by ERR, so if it is an ERR field and the target entity implements that interface, we can call needsSave() instead of actually saving it, and this should guarantee that the reference is saved, even if the host entity is not saving as a new revision. Also stole a useful assertion method from ERR to ensure we get exactly as many node and paragraph revisions as we expect to have.

Interestingly, doing that does seem to fix the bug as well, it no longer fails even with the clone, which means my explanation above isn't 100% correct, and there's something more going on. Either way, we now have it double-fixed.

Would be good if you could do some manual testing with this version as well before I commit it.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Another fun fact: Debugging this too closely also prevents the bug from happening, feels like quantum physics going on here.. affecting an object by observing it..

marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented

Thanks a lot @Berdir for the explanation and looking into the duplicate revisions. It make sense why it wouldn't work by cloning the field. But I still don't understand why it under some circumstance would work. Perhaps I'll continue looking a bit more into it, just out of curiosity.

I've only tested locally yet but the fix for the duplicated revisions looks like a really great improvement. We have a lot of languages installed on our site and this change reduces the number of revisions created by at least half. Even more in cases with nested paragraphs.

Just to get an idea I tested creating a 4 deep nested set of paragraph and the difference in the number of revisions created went from 14 down to 4. That's just really great!

Another fun fact: Debugging this too closely also prevents the bug from happening, feels like quantum physics going on here.. affecting an object by observing it..

I'm really happy I didn't encounter this. :-)

I'll test a bit more on a more complex setup and get back later.

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented

Thanks a lot on working on this.
This looks as a kind of usability blocker for the project I am dealing with now ... hence looking forward for a solid fix/commit of this.
I am going to test this on our project case, too.

marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented

So I've tested the patch on our site and couldn't find any issues with it. The only thing I haven't tested yet is with paragraphs_asymmetric_translation_widgets as mbovan described in Support translation of translatable fields with untranslatable target types - comment #2. I'll try to get to that tomorrow.

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented

This seems working fine also on our side ...

marcusml’s picture

marcusml CreditAttribution: marcusml at Axis Communications AB commented

The changes here doesn't break #3162640: Support translation of translatable fields with untranslatable target types. Based on the manual test steps in that issue.

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented

Well. An update and correction on my side.
We are probably experiencing some issues potentially related/introduced by this issue branch (first updated translation is being accepted and then being set to unpublished, is somehow setting also its source as unpublished, and then causing all the others incoming translation being set "in review", instead of automatically accepted).
I am going to Inspect this further, and make sure there is nothing related to our specific translator provider (https://www.drupal.org/project/tmgmt_xtm).
I am coming back shortly with more details (if confirmed) or a "false alarm" if not ...

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented
FileSize
Screenshot 2021-05-03 at 11.38.12.png487.43 KB

Ok. I was able to better investigate and debug the issues we are experiencing now with this (and not without):

https://git.drupalcode.org/project/tmgmt/-/merge_requests/3/diffs.patch

The base scenario is a Moderated Content, already Published, with different (actually 8) languages translations (with Auto accept finished translations enabled), that gets Re-Published with some updates and Re-Requires all the Translations to the Translation Provider (TMGMT XTM module).

I experience the following scenario, that is not the expected one (expected: all the new Translations coming back as Accepted and Published, like the source):

0. the new Translations requests are sent to the Translation Provider (XTM) when the Source Translation (in English) is Published. So we expect them to come back as Accepted/Published (not in Review)

1. The first translation comes back, as Accepted, and (somehow) sets itself and the source translation (still have to figure out in which exact order) as Not Published …

2. As a consequence of the 1, all the new incoming Translations are getting stuck in Review state.

3. There is a warning appearing on the translation page related to the “Deleted translation” action

(@see the attached screenshots).

It is stil not clear to me what is causing the the first translation getting Accepted as Un-publiched and what is setting un-published the source.
Are you able to re-create this issue/scenario too?
An help on this would be great ...

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented

From further inspection of mine ...
This buggy use case looks happening when the translations are being Accepted in the first round when the Content is not yet published, so that they remain in a not published state.
Once the Content is Published ... then Re-Published and a new Translations requests are triggered the first one coming back is saved as "Not Published", and apparently this also makes the Source Translation being turned on "Un-Published", causing the scenario described in my previosu #16 comment ...

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented
Related issues: +#3212524: Logic to set Auto Accepted Translations Publication Status (accordingly to the Source)

Ok, good news and a meaning full update from me on this.
I discovered that my above described issues are generated by what I assume is a parallel Bug of this 8.x-1.12 of TMGMT in case of Translations requests Auto Accepted but upon a Source that has already been Published.
In that case the $data['#moderation_state'] is not being defined ... as it should to implement a logic to save the Accepted translations in the appropriate default moderation state (matching the Source Moderation State).

I opened this specific and detailed issue on that: https://www.drupal.org/project/tmgmt/issues/3212524
providing a working patch, that doesn't conflict with this issue diffs.

Hence I could say this 3207993-paragraphs-translations branch is working fine for me (again).

cenoura’s picture

cenoura CreditAttribution: cenoura as a volunteer commented

I've used the patch from MR !3 and confirm that it works. Content is being saved again on the paragraphs.
Can we mark this as RTBC?
Thanks!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

  • Berdir committed 9c9a339 on 8.x-1.x authored by marcusml 
    Issue #3207993 by marcusml, itamair: Paragraphs translations aren't...
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Version: 8.x-1.12 » 8.x-1.x-dev
Status: Reviewed & tested by the community » Fixed

Merged now.

Status: Fixed » Closed (fixed)

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

paulmartin84’s picture

paulmartin84 CreditAttribution: paulmartin84 at Inviqa commented

I've spotted an issue with the above fix, I've opened a separate issue here https://www.drupal.org/project/tmgmt/issues/3370882
Basically the above is calling $target_entity->needsSave(); which is a getter rather than a setter, I believe it should be calling $target_entity->setNeedsSave(TRUE);