EntityAdapter should be instantiated per language

Sounds bad indeed - just feels a bit weird that no test currently catch this ?

Is it possible to narrow down a simple code snippet that breaks ?

Definitely needs tests.

ouch. Agreed this needs tests and feels like a critical. But then, it has no implications right now so maybe major is enough?

>Is it possible to narrow down a simple code snippet that breaks ?
Yep, was about to ask that as well.

Hi,

I created a patch for ContentEntityBase based on the specification in this issue.

Hope it helps.

Set status to needs review.

$this->typedDataByLangcode or so might be a better name?

You will also need to unset it if changes happen, search for places where typedData is currently set to NULL.

Updated the name to typedDataByLangcode and added resets.

It would properly make sence to keep the typedData name but one could argue that using a different name makes it more clear what to expect and could prevent confusion.

@plach and @fago both said critical, so let's update the priority then.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -278,6 +285,17 @@ protected function clearTranslationCache() {
+  public function getTypedData() {
+    if (!isset($this->typedDataByLangcode[$this->activeLangcode])) {
+      $class = \Drupal::typedDataManager()->getDefinition('entity')['class'];
+      $this->typedDataByLangcode[$this->activeLangcode] = $class::createFromEntity($this);
+    }
+    return $this->typedDataByLangcode[$this->activeLangcode];
+  }
+

The question is, can we rewrite this to use $this->language()->getId()? If so, then we could possibly move this implementation up into Entity.

Then we could use $this->typedData (or rename that to byLangcode, I don't care), but we'd only have one property and one place to unset them.

As a matter of fact, even though the implementation is in Entity, it only works for content entities at the moment anyway, because we have no adapter that works for config entities.

See my last comment in #2416923: Does not translate when is an anonymous user. @plach said there that this might fix the problem mentioned there, but I can no longer reproduce that bug on HEAD in my contrib module.

Given that, I think we are still lacking an actual problem caused by this that would qualify this being critical?

+1 for not having two separate properties in Entity and ContentEntityBase.

Yeah, agreed, but I think we should either try to only have one implementation of getTypedData() then, or if that is not possible for some reason (it should be), then we should keep the structure consistent and make them both an array, with a hardcoded language key in the base class?

+1 to #18 too :-)

Setting to needs work then.

I reworked the patch so Entity->getTypedData now uses a keyed array instead.

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -43,11 +43,11 @@
   +   * @var array
   

   We try use something like \Drupal\Core\TypedData\ComplexDataInterface[] whenever we know what the array contains.

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -535,11 +535,12 @@ public function toArray() {
   +    $lang_code = $this->language()->getId();
   

   And we usually don't shorten variable names like that, I'd suggest $language_code instead. ($langcode is also pretty common and kind of violates what I just said)

I made the changes you suggested.

If you could provide a failing test, I will have a look at it. :)

Setting to needs work for tests :)

Latest approach seems right indeed - since the translated entities are cloned, no need to keep arrays of TypedDataadapters keyed by language in each of them.

Looks RTBC to me

What is the relationship between this and

  /**
   * Magic method: Implements a deep clone.
   */
  public function __clone() {
    // Avoid deep-cloning when we are initializing a translation object, since
    // it will represent the same entity, only with a different active language.
    if (!$this->translationInitialize) {
      // The translation is a different object, and needs its own TypedData object.
      $this->typedData = NULL;
      $definitions = $this->getFieldDefinitions();
      foreach ($this->fields as $name => $values) {

in ContentEntityBase... I noticed this hunk in #2227227: FieldTypePluginManager cannot instantiate FieldType plugins, good thing TypedDataManager can instantiate just about anything

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 42083a1 and pushed to 8.0.x. Thanks!

Yay, thanks all!