ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity

Actually the problem occurs as follow:
1. Create an entity
2. Add an translation to it
3. Clone the entity in the target translation language
4. From now each field on each translation on the clone will get the entity in the new translation when getEntity is called on a field

The problem relies in ContentEntityBase::_clone
$this->fields[$name][$langcode]->setContext($name, $this->getTypedData());
should be
$this->fields[$name][$langcode]->setContext($name, $this->getTranslation($langcode)->getTypedData());

Attaching the fix.

And here the scenario from #1 with the fix I attached in the previous comment.

And here a dedecated test with the fix, unfortunatelly after I've wrote the test I've noticed that I have to rewrite ContentEntityBase::__clone to allow each field on each translation to have the correct reference ot the right entity in the right translation.

test again.....

+++ b/core/modules/system/src/Tests/Entity/ContentEntityCloneTest.php
@@ -0,0 +1,63 @@
+    foreach (array_keys($clone->getTranslationLanguages()) as $langcode) {

also it needs to assert count(translations) after cloning

@andypost:
Added an assertion for equal translation languages.
Thanks.

So, I've also noticed the problem is a bit bigger... When calling ContentEntityBase::getTranslatedField for an untranslatable field, we first create the field for the default language and then make a reference to it for the another language. This is also wrong, because even if untranslatable the field has a different context. Added a test also for this case.

So the problem is that we used to use only one field for untranslatable fields and now I've changed that, that each translation has a different field. We need some kind of synchronization here or we decide to still use the same approach but then need to extend FieldItemList::getEntity to return the entity in the right translation according to FieldItemList::getLangcode.

Isn't this at least major ?

Was waiting for an approval before setting it as major....

So I decided to keep only one field instance for non translatable fields as it is now done in HEAD.
Made the needed changes, so that after a new translation was added and cloned the non translatable field will still have only reference to the entity in the default translation.

ContentEntityBase::getFields now has two additional parameters : $include_translatable and $include_non_translatable, both set to TRUE as default.

ContentEntityStorageBase::invokeFieldMethod now makes use of the extended functionality of ContentEntityBase::getFields and invokes field methods for non translatable fields ONLY ONCE and ONLY on the default entity translation.

Thought setting the context during cloning is not necessary, but it looks like it is... So bringing it back again.

The previous patch failed, because both tests for entity references (TermTranslationFieldViewTest and EntityReferenceFieldTranslatedReferenceViewTest) have created a non translatable entity reference field on the node and were expecting to see the translated referenced entity on the translated node view....

Now when you request a non translatable field in ContentEntityBase::getTranslatedField, the field will always have as parent the entity in the original language. This leads to the problem with referenced entities.

So with my changes you still can see the translated referenced entity on the referer view, but for that to happen you should make the entity reference field translatable.

@yched:
I need your comment / opinion about this.....

This looks like its doing pretty deep changes in the multilingual fields logic, so it's probably going to need @plach's approval :-)

Can we get an updated summary detailing
- the issue with more than a "quick summary" ;-)
- the proposed fix,
- and how that fix changes the overall entity/translation/field operations flow ?

Other than that, on a first quick look, not a fan of the getFields($optional_bool, $optional_bool, $optional_bool) DX. Three boolean flags make it very hard to understand what some calling code actually does.

Mh I don't get that entirely. An issue is blocked, but why does that justify a criticalness? The other issue is major as well.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -520,10 +521,12 @@ public function set($name, $value, $notify = TRUE) {
+      if (($include_computed || !$definition->isComputed()) &&
+        (($include_translatable && $include_non_translatable) || ($include_translatable && $definition->isTranslatable()) || ($include_non_translatable && !$definition->isTranslatable()))
+      ) {

This seriously needs some docs

Can you expand on why this is critical?
The linked issue is a major.

This is critical as it feels that it could easily lead to data loss.

updated the issue summary

Here again the patch, I removed the two additional booleans from the ContentEntityBase::getFields and introduced instead a new function ContentEntityBase::getTranslatableFields.

P.S.
It is really strange, that the patch does not run on the test bot and also not on my command line.. But it works when running the tests through the browser....

Thanks, this is one of my top focuses for the upcoming days: I'll review it soon.

Would be nice to get a test-only result so we can see the bug in action. Also picked a couple of nits.

@alexpott, @catch, @effulgentsia, @webchick and I discussed this issue and agreed that it should remain critical based on #28, which says:

the translations are clones and therefor affected by this issue here. When the entity is saved, field values are taken from the wrong translation!

If this results in the corrupted translation data being saved to the database, then the issue is critical because of the data integrity problem. (If the data were only corrupted for the current request, without being saved to the database, then the issue would be major.)

We weren't sure about the code snippet in #28, which at first glance appears even worse:

$node = Node::create();
$node->save();
$translation = $node->getTranslation('de');
$translation->save();

$node = Node::load($node->nid);
$node->validate(); // => Fails

Does the failed validation problem on the last line occur permanently, or is it only within the current request?

The bug here isn't directly causing data to be saved, but code might call it on entities that are then saved, for example, during validation. And then your data is messed up.

#2534512: EntityChangedConstraint needs to compare changed time of all translations for example "just" wants to get the last changed date of all translations. And ends up changing the value so the saved entity then has the same changed date in translations and is saved like that. That's what's causing the test fails there. And considering the impressive amount of effort that @mkalkbrenner put into changing exactly, I'm sure this is a very annoying bug for him :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -405,10 +405,21 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
   +      // Call the field methods for non translatable fields only once and on the
   +      // default entity translation.
   

   This comment is quite confusing, we should try to restructure it and better explain *why* we are doing what when.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -405,10 +405,21 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
   +        foreach ($translation->getFields() as $name => $items) {
   +          // call_user_func_array() is way slower than a direct call so we avoid
   +          // using it if have no parameters.
   +          $result[$langcode][$name] = $args ? call_user_func_array([$items, $method], $args) : $items->{$method}();
   +        }
   +      }
   +      else {
   +        foreach ($translation->getTranslatableFields() as $name => $items) {
   +          // call_user_func_array() is way slower than a direct call so we avoid
   +          // using it if have no parameters.
   +          $result[$langcode][$name] = $args ? call_user_func_array([$items, $method], $args) : $items->{$method}();
   +        }
   

   Can we avoid the code duplication here but just getting a list of fields in the if/else and then loop over them after that?

   Do we need this method on the entity or could we also do it in the storage? Then we could give maybe give it maybe a clearer name and do the if/else logic inside? $this->getFieldsToInvoke($translation) as $field) or so?

I'll send @fago a quick mail to tell him about this issue, I'm not sure if he has seen it yet. Doesn't feel like something that I can RTBC myself :)

@berdir:
Thank you for the review. I took your suggestions into account. The new getFieldsToInvoke method is only needed in the storage, as it is too much specific for this case. I do not see any reason to move it to the entity.

The fix looks reasonable to me, but yeah, as said earlier, it would really be best if @plach or @fago vetted this.

Not fully convinced by the separate getFieldsToInvoke() method, this could be directly inlined as a one-line ternary (plus the comment) within invokeFieldMethod(), and would IMO make the overall logic more fluent to read ?

Thanks to @berdir and @yched the logic got now really easy to read and follow.

So now we wait for the approval of @plach and/or @fago.

Sorry for the delay, I was on holiday in the past days and I was hoping to have a good internet connection, but this wasn't the case :(

Anyway, the last patch makes sense to me, I was able to find only a minor issue that can be fixed on commit.

@hchonov++

+++ b/core/modules/system/src/Tests/Entity/ContentEntityCloneTest.php
@@ -0,0 +1,70 @@
+    // Create some test entities.

Wrong inline comment: we are creating a single comment.

Committed 0d06b25 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Entity/ContentEntityCloneTest.php b/core/modules/system/src/Tests/Entity/ContentEntityCloneTest.php
index da70c2f..77d1603 100644
--- a/core/modules/system/src/Tests/Entity/ContentEntityCloneTest.php
+++ b/core/modules/system/src/Tests/Entity/ContentEntityCloneTest.php
@@ -40,7 +40,7 @@ protected function setUp() {
   public function testFieldEntityReferenceAfterClone() {
     $user = $this->createUser();
 
-    // Create some test entities.
+    // Create a test entity.
     $entity = EntityTestMul::create([
       'name' => $this->randomString(),
       'user_id' => $user->id(),
diff --git a/core/modules/taxonomy/src/Tests/TaxonomyTranslationTestTrait.php b/core/modules/taxonomy/src/Tests/TaxonomyTranslationTestTrait.php
index 79e3fed..4b828cf 100644
--- a/core/modules/taxonomy/src/Tests/TaxonomyTranslationTestTrait.php
+++ b/core/modules/taxonomy/src/Tests/TaxonomyTranslationTestTrait.php
@@ -78,6 +78,10 @@ protected function enableTranslation() {
 
   /**
    * Adds term reference field for the article content type.
+   *
+   * @param bool $translatable
+   *   (optional) If TRUE, create a translatable term reference field. Defaults
+   *   to FALSE.
    */
   protected function setUpTermReferenceField($translatable = FALSE) {
     $handler_settings = array(

Fixed #41 and an omission on commit.

This apparently introduced a nasty regression and it looks like we are missing test coverage for that scenario.

See #2540556: Translated paragraphs is not displayed in the correct translation, this broke tests in paragraphs. I reproduced it manually with an entity reference to nodes as well:

Viewing a german node, with a translated paragraph and referencing a node that also has a german translation, both the paragraph and the node is displayed in the EN version.:

We have some test coverage for this in EntityReferenceFieldTranslatedReferenceViewTest but that uses a translatable field. Which is actually a rather uncommon case when you are displaying translated references, since the reference usually doesn't change.

I'm not sure if I fully understand the problem, but have a look at the following line in EntityReferenceFormatter:getEntitiesToView():

$parent_entity_langcode = $items->getEntity()->language()->getId();

Because $items is a non-translatable field, it points to the original translation of the parent entity, so that's en. Somehow, we need to know there that the entity that we are displaying is actually being displayed in german. And i have no idea how to get that information now. We can *not* rely on the current content language, since we want to display the reference in the same language as the parent entity, if possible. There's a second bug here, and that's that it should use getTranslationFromContext(), to fall back to the best possible available translation, but is unrelated to this problem.

I'll open a separate issue for this tomorrow.

This roughly fixes the issue here. It's not pretty, but not completely nonsensical: you may want to force a language different from the parent entity for some reason.

Opened #2543258: Entity references of untranslatable fields are not displayed in the correct translation.

This was expected and I also mentioned it in #22

So with my changes you still can see the translated referenced entity on the referer view, but for that to happen you should make the entity reference field translatable.