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!
Comment | File | Size | Author |
---|---|---|---|
#23 | 3009615-23.patch | 3.03 KB | penyaskito |
|
Comments
Comment #2
dpolant CreditAttribution: dpolant commentedComment #3
dpolant CreditAttribution: dpolant commentedNew patch with hopefully passing tests.
Comment #5
nicxvan CreditAttribution: nicxvan commentedI 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.
Comment #6
dpolant CreditAttribution: dpolant commentedRefined patch a bit. Got failing tests to pass but to many functional ones to test locally. Let's see if it is green ..
Comment #7
dpolant CreditAttribution: dpolant commentedComment #8
penyaskitoThanks 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.
Comment #9
dpolant CreditAttribution: dpolant commentedLooks 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.
Comment #10
penyaskitoQueuing #6 for those other environments too.
Comment #11
penyaskitoThe 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.
Comment #12
penyaskitoHey @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?
Comment #13
dpolant CreditAttribution: dpolant commentedYes, 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?
Comment #14
penyaskitoI'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.
Comment #15
penyaskito@dpolant I'll explore making that a setting, I will let you know.
Comment #16
dpolant CreditAttribution: dpolant commentedThanks penyaskito - in the meantime here is a slightly updated patch with a better empty check.
Comment #17
dpolant CreditAttribution: dpolant commentedComment #18
penyaskito@dpolant: Not sure if related, but worth to take a look: #3026575: When a field is deleted and the translation is redone, the translation keeps the deleted fields
Comment #19
Piotr PakulskiUpdated version of patch for v2.12, because was not applying.
Comment #21
Piotr PakulskiComment #22
penyaskito@paqu83 Is this patch still needed?
Comment #23
penyaskitoRerolled.
Comment #24
penyaskitoI 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.
Comment #25
penyaskitoNot closing yet.