This issue is essentially the same as https://www.drupal.org/project/lingotek/issues/2975448, however I think the route we followed to a solution there is not correct.

Reposting the steps to reproduce from 2975448:

1. Create an English (default language) node with the following structure:
Node
i. Paragraph A
ii. Paragraph B
iii. Paragraph C
2. Paragraphs A, B and C must reference another paragraph via a paragraph reference field (Note I am not so sure about this one)
3. Go to /admin/lingotek/manage/node. Upload the new node to Lingotek.
4. Check box next to new node and under bulk doc management, select “Request all translations” and click Execute.
5. Check box next to new node and under bulk doc management, select “Check progress of all translations” and click “Execute” until translations are ready for download.
6. Edit the English node. Remove paragraph A and add paragraph D so the structure is now:
Node
i. Paragraph D
ii. Paragraph B
iii. Paragraph C
7. Go to /admin/lingotek/manage/node. Check the box next to node, select “Download all translations” and click “Execute”.
8. Go back to the English version of the node. It has reverted to
Node
i. Paragraph A
ii. Paragraph B
iii. Paragraph C

In short I don't think setting ERR fields as translatable is the right way to go. We don't actually want different translations to reference different paragraphs or have the potential to have divergent revisions of the same paragraph. For instance if an editor deletes a node in English, we mean for it to delete in all translations. Moreover, we've deemed it too risky to switch to a new and barely supported or possibly unsupported translation model.

Unless I am mistaken, the problem is caused by how Drupal\lingotek\LingotekContentTranslationService::saveTargetData() works, specifically this part:

          elseif ($field_type === 'entity_reference_revisions') {
            $target_entity_type_id = $field_definition->getFieldStorageDefinition()
              ->getSetting('target_type');
            $index = 0;
            foreach ($field_data as $field_item) {
              $embedded_entity_id = $revision->{$name}->get($index)
                ->get('target_id')
                ->getValue();
              /** @var \Drupal\Core\Entity\RevisionableInterface $embedded_entity */
              $embedded_entity = $this->entityManager->getStorage($target_entity_type_id)
                ->load($embedded_entity_id);
              if ($embedded_entity !== NULL) {
                $this->saveTargetData($embedded_entity, $langcode, $field_item);
                // Now the embedded entity is saved, but we need to ensure
                // the reference will be saved too. Ensure it's the same revision.
                $translation->{$name}->set($index, ['target_id' => $embedded_entity_id, 'target_revision_id' => $embedded_entity->getRevisionId()]);
              }
              ++$index;
            }
          }

The trouble is that $field_data may reference paragraphs that an admin removed from the parent entity at some point after the node was last uploaded. In this case, it will revert the value of the ERR reference fields to what it was in the revision of the parent entity that Lingotek module keeps track of in the metadata upon upload.

I'll provide a patch as a starting point. It fixes the problem by using making sure that saveTargetData acts on the correct deltas instead of assuming what's coming from the download.

Please let me know what you think of this assessment/fix.

Thanks!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpolant created an issue. See original summary.

dpolant’s picture

Status: Active » Needs review
dpolant’s picture

New patch with hopefully passing tests.

Status: Needs review » Needs work

The last submitted patch, 3: 3009615--revision-tracking-fix-8x2x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nicxvan’s picture

I think I'm experiencing the same issue, would you be willing to review with me to make sure. Then I'd be willing to test the patch.

I'm on the drupal slack with the same username.

dpolant’s picture

Refined patch a bit. Got failing tests to pass but to many functional ones to test locally. Let's see if it is green ..

dpolant’s picture

Status: Needs work » Needs review
penyaskito’s picture

Thanks for keeping digging at this!

I'm happy to commit this as far as everything is green, but "The trouble is that $field_data may reference paragraphs that an admin removed from the parent entity at some point after the node was last uploaded.".

How a paragraph was deleted? Isn't that marked as deleted, but still there as it's an older kept revision?

Our idea was that when you are translating your content, we are creating a translation to the revision that was uploaded. After D8 launched, this meant a new revision every time (so you cannot just add a translation to an existing revision, and a new translation is created).

I think this is what it's fixed here in your patch.

Just for the sake of curiosity, let's see without the patches to ERR and core.

dpolant’s picture

Status: Needs review » Needs work

Looks like one fail on 8.5, I can look into that.

Re: How was paragaraph deleted?
I don't think it's actually deleting the paragraph but rather removing it from the value of the field reference. The problem was that the payload coming in from Lingotek when we download was trying to set a reference back to that paragraph on the translation.

Thanks for confirming the direction on this patch. We're sending it through UAT soon on our end so I'll keep you posted with any findings.

penyaskito’s picture

Queuing #6 for those other environments too.

penyaskito’s picture

The failure in #8 is the same than #2975448-37: Downloading translations reverts changes to source language paragraph references, somehow expected. I expect #6 to pass on that as Core should be patched on that test.

penyaskito’s picture

Status: Needs work » Postponed (maintainer needs more info)

Hey @dpolant, any news on this? I'm still wondering: if I edited the source and added/removed paragraphs without re-uploading, the downloaded translation should contain all the original paragraphs, or it could be incomplete.

E.g. revision 1 has A, B, C and its translation requested; revision 2 has A, D, C.

If I download the translation, with the latest patch I think it would reference A (translated), D (untranslated), C (translated).

Is that the result you expect?

dpolant’s picture

Yes, we'd expect it to be A, D, C after download in your example.

What are next steps for getting this committed? Are the test failures something we can deal with now or are we still waiting on core?

penyaskito’s picture

I'm actually surprised they didn't fail in other versions, because having A, B, C downloaded and visible in the translation is actually what I would expect and what we were (supposedly) testing. IMHO it's exactly what I would expect, if I uploaded Rev2 and I download a translation, Rev2(T) is what I get, not Rev3 which was created (and not uploaded) afterwards.

This would be a big impact behaviour change, not sure if all customers are expecting the same.
I have to discuss this with other stakeholders before moving this further.

penyaskito’s picture

Status: Postponed (maintainer needs more info) » Needs work

@dpolant I'll explore making that a setting, I will let you know.

dpolant’s picture

Thanks penyaskito - in the meantime here is a slightly updated patch with a better empty check.

dpolant’s picture

Status: Needs work » Needs review
penyaskito’s picture

Piotr Pakulski’s picture

Updated version of patch for v2.12, because was not applying.

Status: Needs review » Needs work

The last submitted patch, 19: 3009615-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Piotr Pakulski’s picture

Version: 8.x-2.x-dev » 8.x-2.12
Status: Needs work » Needs review
FileSize
1.01 KB
3.03 KB
penyaskito’s picture

@paqu83 Is this patch still needed?

penyaskito’s picture

Status: Needs review » Closed (outdated)

I this this has been already fixed in the light of some other efforts.

If you could confirm that the problem cannot be reproduced, I will make an empty commit to credit those that worked in this issue, but I don't think there's anything else needed here.

penyaskito’s picture

Status: Closed (outdated) » Postponed (maintainer needs more info)

Not closing yet.