Various EntityNG and TypedData API improvements

#3: d8_typed_data_improvements.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -77,20 +77,17 @@ public function __construct($entityType) {
-    $entity = new $this->entityClass(array(), $this->entityType);
+    // We have to determine the bundle first.
+    $bundle = $this->bundleKey ? $values[$this->bundleKey] : FALSE;
+    $entity = new $this->entityClass(array(), $this->entityType, $bundle);
 
-    // Make sure to set the bundle first.
-    if ($this->bundleKey) {
-      $entity->{$this->bundleKey} = $values[$this->bundleKey];
-      unset($values[$this->bundleKey]);
-    }
     // Set all other given values.
     foreach ($values as $name => $value) {
       $entity->$name = $value;

Is there reason we can't pass $values to the entity? If that doesn't work then there isn't much use in having that argument at all, is there?

Ah, saw it below, the argument expects a different format of the values. Would it make sense to convert them instead of adding them through magic setters?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -109,19 +106,20 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
       if ($load_revision) {
-        field_attach_load_revision($this->entityType, $queried_entities);
+        field_attach_load_revision($this->entityType, $bc_entities);
       }
       else {
-        field_attach_load($this->entityType, $queried_entities);
+        field_attach_load($this->entityType, $bc_entities);

Every time I see this I think we should kill field_attach_load_revision() in favor of a isDefaultRevision() check in field_attach_load()...

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -0,0 +1,364 @@
+ * @file
+ * Definition of Drupal\Core\Entity\EntityBCDecorator.

Should be Contains now ;)

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -0,0 +1,364 @@
+   * Allows directly writing to the plain values, as done in Drupal 7.

Not sure that comment is grammatically corrt.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -0,0 +1,364 @@
+
+  /**
+   * Magic method.
+   */
+  public function __isset($name) {
+    $value = $this->__get($name);
+    return isset($value);
+  }
+
+  /**
+   * Magic method.

Not sure what the coding standards say about documentation magic methods, but probably not "Magic method" :)

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -190,4 +191,21 @@ public function isDefaultRevision($new_value = NULL);
+   *
+   * @see \Drupal\Core\Entity\EntityInterface::getOriginalEntity()
+   *
+   * @return \Drupal\Core\Entity\EntityInterface
+   */
+  public function getBCEntity();
+
+  /**
+   * Removes any possibly (backward compatibility) decorator in use.
+   *
+   * @see \Drupal\Core\Entity\EntityInterface::getBCEntity()
+   *
+   * @return \Drupal\Core\Entity\EntityInterface

@return should be above @see and have a short one line description.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -26,6 +26,13 @@
   /**
+   * Local cache holding the value of the bundle field.
+   *
+   * @var string
+   */
+  protected $bundle = FALSE;

This makes sense. What happens if you do something like $node->type = 'your-now-something-else';?

I guess that's not really supported anyway but if it isn't, then we should prevent it from happening. Make the bundle field read-only or so?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -318,23 +386,28 @@ public function updateOriginalValues() {
+    // Allow the EntityBCDecorator to directly access the values and fields.
+    // @todo: Remove once the EntityBCDecorator gets removed.
+    if ($name == 'values' || $name == 'fields') {
+      return $this->$name;

Is there a reason for doing it like this instead of providing temporary helper methods for that? Needs to be removed either way and this is quite the ugly :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.phpundefined
@@ -131,7 +131,7 @@ function testEntityLanguageMethods() {
-    // Try to get an untranslatable value from a translation in strict mode.
+    // Try to get an unstranslatable value from a translation in strict mode.
     try {
       $field_name = 'field_test_text';
       $value = $entity->getTranslation($this->langcodes[1])->get($field_name);
@@ -141,7 +141,7 @@ function testEntityLanguageMethods() {

@@ -141,7 +141,7 @@ function testEntityLanguageMethods() {
       $this->pass('Getting an untranslatable value from a translation in strict mode throws an exception.');
     }
 
-    // Try to get an untranslatable value from a translation in non-strict
+    // Try to get an unstranslatable value from a translation in non-strict
     // mode.
     $entity->set($field_name, array(0 => array('value' => 'default value')));
     $value = $entity->getTranslation($this->langcodes[1], FALSE)->get($field_name)->value;
@@ -156,7 +156,7 @@ function testEntityLanguageMethods() {

@@ -156,7 +156,7 @@ function testEntityLanguageMethods() {
       $this->pass("Setting a translation for an invalid language throws an exception.");
     }
 
-    // Try to set an untranslatable value into a translation in strict mode.
+    // Try to set an unstranslatable value into a translation in strict mode.

A search & replace gone wrong? ;)

Changed look good generally, but it's quite as long patch, containing various different things which makes it hard to review.

Instead of unsetting the empty fields, should we fix it so they are not set empty in the first place?

#19: d8_typed_data_improvements.patch queued for re-testing.

here is the conflict when trying to reroll:

  public function buildContent(array $entities = array(), $view_mode = 'full', $langcode = NULL) {
    // Allow modules to change the view mode.
    $context = array('langcode' => $langcode);

    $view_modes = array();
    $displays = array();

    foreach ($entities as $entity) {
      // Remove previously built content, if exists.
      $entity->content = array();

      drupal_alter('entity_view_mode', $view_mode, $entity, $context);
      $entity->content['#view_mode'] = $view_mode;
<<<<<<< HEAD
      $view_modes[$view_mode][$entity->id()] = $entity;

      $bundle = $entity->bundle();

      // Load the corresponding display settings if not stored yet.
      if (!isset($displays[$view_mode][$bundle])) {
        // Get the display object to use for rendering the entity..
        $display = entity_get_render_display($entity, $view_mode);

        // Let modules alter the display.
        // Note: if config entities get a static cache at some point, the
        // objects should be cloned before running drupal_alter().
        $display_context = array(
          'entity_type' => $this->entityType,
          'bundle' => $bundle,
          'view_mode' => $view_mode,
        );
        drupal_alter('entity_display', $display, $display_context);

        $displays[$view_mode][$bundle] = $display;
      }

      // Assigning weights to 'extra fields' is done in a pre_render callback.
      $entity->content['#pre_render'] = array('_entity_view_pre_render');
      $entity->content['#entity_display'] = $displays[$view_mode][$bundle];
    }

    // Prepare and build field content, grouped by view mode.
    foreach ($view_modes as $view_mode => $view_mode_entities) {
      $call_prepare = array();
      // To ensure hooks are only run once per entity, check for an
      // entity_view_prepared flag and only process relevant entities.
      foreach ($view_mode_entities as $entity) {
        if (empty($entity->entity_view_prepared) || $entity->entity_view_prepared != $view_mode) {
          // Add this entity to the items to be prepared.
          $call_prepare[$entity->id()] = $entity;

          // Mark this item as prepared for this view mode.
          $entity->entity_view_prepared = $view_mode;
        }
      }

      if (!empty($call_prepare)) {
        field_attach_prepare_view($this->entityType, $call_prepare, $displays[$view_mode], $langcode);
        module_invoke_all('entity_prepare_view', $call_prepare, $this->entityType);
      }

      foreach ($view_mode_entities as $entity) {
        $entity->content += field_attach_view($this->entityType, $entity, $displays[$view_mode][$entity->bundle()], $langcode);
=======
      $prepare[$view_mode][$entity->id()] = $entity;
    }

    // Prepare and build field content, grouped by view mode.
    foreach ($prepare as $view_mode => $prepare_entities) {
      field_attach_prepare_view($this->entityType, $prepare_entities, $view_mode, $langcode);
      module_invoke_all('entity_prepare_view', $prepare_entities, $this->entityType);

      foreach ($prepare_entities as $entity) {
        $entity->content += field_attach_view($this->entityType, $entity, $view_mode, $langcode);
>>>>>>> 19
      }
    }
  }

I gave it a try:

  public function buildContent(array $entities = array(), $view_mode = 'full', $langcode = NULL) {
    // Allow modules to change the view mode.
    $context = array('langcode' => $langcode);

    $view_modes = array();
    $displays = array();

    foreach ($entities as $entity) {
      // Remove previously built content, if exists.
      $entity->content = array();

      drupal_alter('entity_view_mode', $view_mode, $entity, $context);
      $entity->content['#view_mode'] = $view_mode;
      $view_modes[$view_mode][$entity->id()] = $entity;

      $bundle = $entity->bundle();

      // Load the corresponding display settings if not stored yet.
      if (!isset($displays[$view_mode][$bundle])) {
        // Get the display object to use for rendering the entity..
        $display = entity_get_render_display($entity, $view_mode);

        // Let modules alter the display.
        // Note: if config entities get a static cache at some point, the
        // objects should be cloned before running drupal_alter().
        $display_context = array(
          'entity_type' => $this->entityType,
          'bundle' => $bundle,
          'view_mode' => $view_mode,
        );
        drupal_alter('entity_display', $display, $display_context);

        $displays[$view_mode][$bundle] = $display;
      }

      // Assigning weights to 'extra fields' is done in a pre_render callback.
      $entity->content['#pre_render'] = array('_entity_view_pre_render');
      $entity->content['#entity_display'] = $displays[$view_mode][$bundle];
    }

    // Prepare and build field content, grouped by view mode.
    foreach ($view_modes as $view_mode => $view_mode_entities) {
      field_attach_prepare_view($this->entityType, $view_mode_entities, $displays[$view_mode], $langcode);
      module_invoke_all('entity_prepare_view', $view_mode_entities, $this->entityType);

      foreach ($view_mode_entities as $entity) {
        $entity->content += field_attach_view($this->entityType, $entity, $displays[$view_mode][$entity->bundle()], $langcode);
      }
    }
  }

I was not sure about the new second half of the OR:

if (empty($entity->entity_view_prepared) || $entity->entity_view_prepared != $view_mode) {

I did mainly a visual review, architecture wise I trust fully fago (there were also quite some discussions in IRC about a lot of this stuff) and the tests pass :)
This are mainly nitpicks, thus I'll make the mentioned changes asap.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -77,20 +77,17 @@ public function __construct($entityType) {
    *   A new entity object.
    */
   public function create(array $values) {
-    $entity = new $this->entityClass(array(), $this->entityType);
+    // We have to determine the bundle first.
+    $bundle = $this->bundleKey ? $values[$this->bundleKey] : FALSE;
+    $entity = new $this->entityClass(array(), $this->entityType, $bundle);

@@ -150,13 +148,14 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
...
-      $records[$id] = $entity;
+      $bundle = $this->bundleKey ? $record->{$this->bundleKey} : FALSE;
+      // Turn the record into an entity class.
+      $records[$id] = new $this->entityClass($values, $this->entityType, $bundle);

@@ -48,30 +48,38 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
...
+      }
+      $entity = new $this->entityClass($values, $this->entityType);
+      $records[$id] = $entity;
+    }
     return $records;

Not sure if this collides with the coding standards: When calling class constructors with no arguments, always include parentheses. I think the return value from $this->entityClass() should go into a variable first.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
...
+ * Allows using entities converted to the new Entity Field API with the Drupal 7
+ * way of accessing ways fields or properties.

I think "ways" should be removed.

+  /**
+   * Forwards the call to the decorated entity.
+   */
+  public function label($langcode = NULL) {
+    return $this->decorated->label();
+  }

Shouldn't this pass the $langcode to the method of the decorated entity too?

+  /**
+   * Forwards the call to the decorated entity.
+   */
+  public function isDefaultRevision($new_value = NULL) {
+    return $this->decorated->isDefaultRevision();
+  }

Shouldn't this pass the $new_value to the method of the decorated entity too?

@@ -2076,9 +2076,9 @@ protected function createTypedData($definition, $value = NULL, $context = array(
...
+    $this->assertTrue(!empty($definition['type']), $definition['type'] . ' data definition was returned.');
     // Assert that the correct type was constructed.
-    $this->assertEqual($data->getType(), $type, $definition['label'] . ' object returned type.');
+    $this->assertEqual($data->getType(), $type, $definition['type'] . ' object returned type.');

We should use format_string() here, right?

Okay, forget about $this->entityClass. I wasn't aware this is a class variable and thus already matches to the coding standards...

Other changes are made in the updated patch.

I did mainly a visual review, architecture wise I trust fully fago (there were also quite some discussions in IRC about a lot of this stuff) and the tests pass :)

Same here... So here comes my nitpick review. Will provide a patch too afterwards.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -126,4 +127,53 @@ public function getExportProperties() {
+   * Implements Drupal\Core\Entity\EntityInterface::getBCEntity().

We shouldn't have these full namespaces in the method docblocks. Should be simply "Implements EntityInterface::getBCEntitiy()." etc.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -0,0 +1,368 @@
+   * Forwards the call to the decorated entity.

Wait... I read that before somewhere :P.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -190,4 +191,23 @@ public function isDefaultRevision($new_value = NULL);
+   * Removes any possibly (backward compatibility) decorator in use.

"possibly" is wrong here (should be "possible", no?).

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.phpundefined
@@ -96,24 +91,19 @@ public function setValue($values) {
+    elseif ($values === array()) {

That looks weird. I would be happier if it was a is_array($values) && empty($values) even if that's longer.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataFactory.phpundefined
@@ -25,28 +26,36 @@ class TypedDataFactory extends DefaultFactory {
+   *   data object implementing either theListInterface or the

missing whitespace (the ListInterface)

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -2,11 +2,12 @@
 /**
  * @file
- * Definition of Drupal\Core\TypedData\TypedDataManager.
+ * Contains \Drupal\Core\TypedData\TypedDataManager.
  */

I still don't understand the policy for this fully. Do we use a backslash in the @file docblock now or not? Most of core doesn't but than again there are about 100 occurrences with leading backslash.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -70,42 +85,124 @@ public function createInstance($plugin_id, array $configuration) {
+   *   The name of the property to instantiate, or the delta of an list item.

Should be "a list item" no? I wish my english was better :P

+++ b/core/modules/text/lib/Drupal/text/TextProcessed.phpundefined
@@ -104,21 +69,28 @@ public function getValue($langcode = NULL) {
-   * Implements TypedDataInterface::setValue().
+   * Implements \Drupal\Core\TypedData\TypedDataInterface::setValue().

Why? It was correct before.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerNG.phpundefined
@@ -0,0 +1,26 @@
+ * Definition of Drupal\translation_entity\EntityTranslationControllerNG.

Should be 'Contains'

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1080,4 +1081,52 @@ public function setOriginalID($id) {
+   * Implements Drupal\Core\Entity\EntityInterface::getOriginalEntity().
...
+   * Implements \Drupal\Core\TypedData\ContextAwareInterface::getName().

Getting more and more confused about our policies regarding namespaces in docblocks ...

We still haven't decided on exactly how we reference namespaced classes, so I would just leave that stuff alone except for the Contains/Definition of part.

We still haven't decided on exactly how we reference namespaced classes, so I would just leave that stuff alone except for the Contains/Definition of part.

Alright, that makes it easier.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -26,6 +26,13 @@
+   * Local cache holding the value of the bundle field.
+   *
+   * @var string
+   */
+  protected $bundle = FALSE;

@var data type does not match default value, use "@var string|FALSE" instead.

+++ b/core/lib/Drupal/Core/TypedData/ContextAwareInterface.php
@@ -23,36 +23,48 @@
+   * @return \Drupal\Core\TypedData\ComplexDataInterface|\Drupal\Core\TypedData\ListInterface

That looks ugly, as you never know what getParent() returns if you don't know much about the object. Splitting that up into getListParent() and getDataParent() only makes the situation more complex, so I guess this is ok.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataFactory.php
@@ -25,28 +26,35 @@ class TypedDataFactory extends DefaultFactory {
+   *   (optional) If a property or list item is to be created, the name of the
+   *   property or the delta of the the list item. Otherwise, leave it empty.

"the the". Also, "leave it empty" sounds weird here. Better use "Otherwise omit the parameter."? Or just don't mention that at all, the parameter is marked as optional, so people know how to use it.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataFactory.php
@@ -25,28 +26,35 @@ class TypedDataFactory extends DefaultFactory {
+   *   ComplexDataInterface. Otherwise, leave it empty.

Same here.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -28,11 +36,18 @@ public function __construct() {
+   *   property or the delta of the the list item. Otherwise, leave it empty.

Same here. And possibly quite a few other places.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
index 3dd810c..56f3873 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php

--- a/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php

Additional test case for setting NULL values form the last patch in #1868274: Improved EntityNG isset() test coverage is missing.

Ok, I do not feel fully qualified to RTBC this, but since we already had some other positive feedback and even the nitpicking comes to an end for this large patch ==> good to go.

I looked over this patch and did not find anything major that would or should block a commit.

The only more general issue I had while reviewing is that a lot of the new (and existing) entity data handling methods are performing very complex operations, but they do not contain any inline code comments that would explain what is being done and more importantly why. Therefore, I stared on a couple of code blocks and tried to mentally decipher what the code is doing, and why, and whether the code is correctly doing what it attempts to do, but in almost all cases, I ended up with "Hrm... whatever, I hope it's ok."

The only other and partially more elaborate comments are actually @todo comments... and most of those are even harder to decipher for someone who's not neck-deep into the code ;)

Could we create a separate, dedicated issue for improving the technical inline code documentation? And/or alternatively, can we make sure to vastly improve the docs through other issues/patches? :)

In any case, I think this is RTBC and we can move forward with this patch.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -126,4 +127,53 @@ public function getExportProperties() {
+  public function getBCEntity() {
...
+  public function getOriginalEntity() {
...
+  public function getName() {
...
+  public function getRoot() {
...
+  public function getPropertyPath() {
...
+  public function getParent() {

Config\Entity only overrides selective Entity\Entity methods thus far - these methods appear to be identical and not overridden in any way; was there any reason to duplicate them?

We can remove them in a follow-up patch though - no reason to hold up the commit for that.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -190,4 +191,23 @@ public function isDefaultRevision($new_value = NULL);
+  public function getOriginalEntity();

This slightly clashes with our current notion of $entity->original during saving/updating, no?

However, if I get this right, then this BC layer is supposed to be removed anyway before release, so I guess the name clash doesn't matter for now.

That said, do we have a "Plan B" in case we do not manage to kill the BC layer before release? Should we perhaps create an issue for such a plan B, tagged with "revisit before release"?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -50,22 +57,69 @@ class EntityNG extends Entity {
+  protected function init() {
+    // We unset all defined properties, so magic getters apply.
+    unset($this->langcode);
+  }

What I do not really understand in the new EntityNG design is how default values of entity properties are handled — e.g., as in this particular case, $this->langcode actually has a default value, which is simply removed by init() and thus entirely lost, no? So unless I'm missing something big, we're losing an essential and required concept of classes.

I guess this question is not really related to this issue/patch though... Do we have an issue for that already?

Thanks!

Follow-ups from #37

• #1877632: Improve comments and clean-up code for EntityNG and the TypedData API
• #1877638: Rename getOriginalEntity() and remove duplicated functions from ConfigEntityBase
• #1877644: Develop a "Plan B" in case BC layer not killed (from: Various EntityNG and TypedData API improvements)

Yes, this needs to go in. It is blocking multiple major/critical issues, so setting to major as well.

Thanks for splitting this into its own issue. Should make the comment conversion much easier to review. Committed/pushed to 8.x.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+  /**
+   * Forwards the call to the decorated entity.
+   */
+  public function access($operation = 'view', \Drupal\user\Plugin\Core\Entity\User $account = NULL) {
+    return $this->decorated->access($account);
+  }

I'm sorry if this has been discussed above (couldn't find anything) or if I'm missing something, but I'm pretty sure this should forward $operation as well, not just $account.

Re-opening for that. Should be a quick fix. Don't know if we need tests, though.

public function __call($method, $args) {
  return call_user_func_array(array($this->decorated, $method), $args);
}

And in the decorated method:
return $this->__call(__FUNCTION__, func_get_args());

Sorry if this has already been explained, but why do we override the new Entity methods in ConfigEntityBase and provide the same implementation?

I say RTBC ;)

I only wanted to create a review for #1877632: Improve comments and clean-up code for EntityNG and the TypedData API, but while cycling through the patch once more, I had a couple of further questions, so I'm posting the review here instead of over there.

Some of them are related to this issue/patch, some others are more generic issues with typed data.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -687,15 +694,17 @@ public function getFieldDefinitions(array $constraints) {
+      if ($bundle && isset($this->entityFieldInfo['bundle map'][$constraints['bundle']])) {
+        $this->fieldDefinitions[$bundle] += array_intersect_key($this->entityFieldInfo['optional'], array_flip($this->entityFieldInfo['bundle map'][$constraints['bundle']]));
+      }

1) What kind of additional optional definitions are added here, and why do they overlap with the bundle definitions? Why are additional values from an optional definition for a bundle key left out and not merged in (due to +=)?

2) $constaints['bundle'] == $bundle, so parts could be simplified here.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -77,20 +77,17 @@ public function __construct($entityType) {
   public function create(array $values) {
...
+    // We have to determine the bundle first.
+    $bundle = $this->bundleKey ? $values[$this->bundleKey] : FALSE;

The comment can be removed. Or perhaps replaced in order to explain why $this->bundleKey is re-evaluated here - isn't exactly this negotiation happening in __construct() of a storage controller already? I.e., isn't $this->bundleKey == $bundle anyway already?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -77,20 +77,17 @@ public function __construct($entityType) {
     // Set all other given values.
     foreach ($values as $name => $value) {
       $entity->$name = $value;

Why are the passed in values to create() set directly as property $name, whereas all other code is using $name->value?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -150,13 +148,14 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
       foreach ($record as $name => $value) {
-        $entity->{$name}[LANGUAGE_DEFAULT][0]['value'] = $value;
+        // Avoid unnecessary array hierarchies to save memory.
+        $values[$name][LANGUAGE_DEFAULT] = $value;
       }

It's not clear why the value assignment does not account for multiple value records.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+ * Allows using entities converted to the new Entity Field API with the Drupal 7
+ * way of accessing fields or properties.

Can we replace "the Drupal 7 way of..." with its actual meaning? And also clarify what the difference to the non-decorated way is?

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+ * Note: We access the protected 'values' and 'fields' properties of the entity
+ * via the magic getter - which returns them by reference for us. We do so, as
+ * providing references to this arrays makes $entity->values and $entity->fields
+ * to references itself as well, which is problematic during __clone() (this is
+ * something that would not be easy to fix as an unset() on the variable is
+ * problematic with the magic getter/setter then).

It's not clear to me what this comment tries to communicate and educate about. The (logical/linguistic) context and topic is missing. It also sounds/feels like the comment rather belongs onto a method's phpDoc block instead of the class level (though not sure).

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+  public function &__get($name) {
+    // Make sure $this->decorated->values reflects the latest values.
+    if (!empty($this->decorated->fields[$name])) {
+      foreach ($this->decorated->fields[$name] as $langcode => $field) {
+        $this->decorated->values[$name][$langcode] = $field->getValue();
+      }
+      // Values might be changed by reference, so remove the field object to
+      // avoid them becoming out of sync.
+      unset($this->decorated->fields[$name]);

Who or what is setting $this->decorated->fields[$name] (the corresponding __set() method does not), and why can this code unset that property without re-instantiating it?

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+    // Allow accessing field values in entity default languages other than
+    // LANGUAGE_DEFAULT by mapping the values to LANGUAGE_DEFAULT.
+    $langcode = $this->decorated->language()->langcode;
+    if ($langcode != LANGUAGE_DEFAULT && isset($this->decorated->values[$name][LANGUAGE_DEFAULT]) && !isset($this->decorated->values[$name][$langcode])) {
+      $this->decorated->values[$name][$langcode] = &$this->decorated->values[$name][LANGUAGE_DEFAULT];
+    }

I don't really understand why this is copying values from one language to another, whereas there are actually no values for the other language?

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+  public function __set($name, $value) {
+    if (is_array($value) && $definition = $this->decorated->getPropertyDefinition($name)) {

$definition seems to be unused? Do we need a hasProperty() method?

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -0,0 +1,368 @@
+  public function __unset($name) {
+    $value = &$this->__get($name);
+    $value = array();
+  }

Why do we unset to an array instead of NULL?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -50,22 +57,69 @@ class EntityNG extends Entity {
+  /**
+   * Gets the typed data type of the entity.
...
+   */
+  public function getType() {
+    return $this->entityType;
+  }

An entity type with the name "number" or any other typed data type name will clash... Shouldn't we prefix these dynamic entity type data types with "entity_" or something?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -105,9 +159,11 @@ protected function getTranslatedField($property_name, $langcode) {
-        $this->fields[$property_name][$langcode] = typed_data()->create($definition, $value, $context);
...
+        $this->fields[$property_name][$langcode] = typed_data()->getPropertyInstance($this, $property_name, $value);

Hm. create() was a lot more clear as a method name here.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -163,10 +215,13 @@ public function getPropertyDefinition($name) {
   public function getPropertyDefinitions() {
-    return entity_get_controller($this->entityType)->getFieldDefinitions(array(
...
+    return $this->fieldDefinitions;

It looks like we're copying the entire property definitions into every entity object? Did we perform benchmarks on memory consumption?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -163,10 +215,13 @@ public function getPropertyDefinition($name) {
+      $this->fieldDefinitions = entity_get_controller($this->entityType)->getFieldDefinitions(array(
+        'entity type' => $this->entityType,
+        'bundle' => $this->bundle,
+      ));

"entity type" with a space instead of a underscore ("entity_type") was a bit unexpected here - also not sure why the arguments are passed as an array instead of two arguments?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -243,11 +307,11 @@ public function getTranslation($langcode, $strict = TRUE) {
+    $translation = typed_data()->create($translation_definition, $fields);
     $translation->setStrictMode($strict);
+    if ($translation instanceof ContextAwareInterface) {
+      $translation->setContext('@' . $langcode, $this);
+    }
     return $translation;

Hm. I've to admit it looks like I missed a couple of issues and commits regarding the TypedData API.... Thus, no idea what a "strict mode" is. But regardless of that, the DX of setContext() appears to be awkward and could really use some love.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -256,89 +320,101 @@ public function getTranslation($langcode, $strict = TRUE) {
+    // Build an array with the translation langcodes set as keys. Empty
+    // translations must be filtered out.
     foreach ($this->getProperties() as $name => $property) {
-      if (isset($this->values[$name])) {
-        $translations += $this->values[$name];
+      foreach ($this->fields[$name] as $langcode => $field) {
+        if (!$field->isEmpty()) {
+          $translations[$langcode] = TRUE;
+        }
+        if (isset($this->values[$name])) {
+          foreach ($this->values[$name] as $langcode => $values) {
+            if ($values && !(isset($this->fields[$name][$langcode]) && $this->fields[$name][$langcode]->isEmpty())) {
+              $translations[$langcode] = TRUE;
+            }
+          }
+        }
       }
-      $translations += $this->fields[$name];
     }

Why are empty translations filtered out?

I think I should be able to create a translation that is empty? E.g., when I purposively want to have an empty translation?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -346,41 +422,40 @@ public function __set($name, $value) {
+    // Else directly read/write plain values. That way, fields not yet converted
+    // to the entity field API can always be directly accessed.
     else {
-      $this->$name = $value;
+      $this->values[$name] = $value;
     }
...
+    else {
+      return isset($this->values[$name]);
     }
...
+    else {
+      unset($this->values[$name]);
     }

I don't really understand why all of this direct munging with values happens in the Entity class instead of the BC decorator?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -346,41 +422,40 @@ public function __set($name, $value) {
+   * Implements the magic method for isset().
    */
   public function __isset($name) {
...
+    if ($this->getPropertyDefinition($name)) {
+      return $this->get($name)->getValue() !== NULL;
     }
...
+   * Implements the magic method for unset.
    */
   public function __unset($name) {
...
+    if ($this->getPropertyDefinition($name)) {
+      $this->get($name)->setValue(NULL);
     }

1) It really looks like we could use a hasProperty($name) method, since all of these conditional checks essentially pass an array around, while a simple Boolean TRUE/FALSE is all that's needed.

2) Is there no isset() method on properties? The comparison against NULL and also unsetting by setting NULL does not look good to me.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -404,13 +479,30 @@ public function createDuplicate() {
+  /**
+   * Overrides Entity::label() to access the label field with the new API.
+   */
+  public function label($langcode = NULL) {
+    $label = NULL;
+    $entity_info = $this->entityInfo();
+    if (isset($entity_info['label_callback']) && function_exists($entity_info['label_callback'])) {
+      $label = $entity_info['label_callback']($this->entityType, $this, $langcode);
+    }
+    elseif (!empty($entity_info['entity_keys']['label']) && isset($this->{$entity_info['entity_keys']['label']})) {
+      $label = $this->{$entity_info['entity_keys']['label']}->value;
+    }
+    return $label;
+  }

Isn't this exactly the same code as Entity::label(), except for a final:

$label = parent::label();
return $label->value;

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityTranslation.php
@@ -235,7 +193,10 @@ public function isEmpty() {
+    // @todo Add a way to set and get the langcode so that's more obvious what
+    // we're doing here.
+    $langocde = substr($this->getName(), 1);
+    return entity_access_controller($this->parent->entityType())->$method($this->parent, $langocde, $account);

1) Typo in "$langocde" ($langcode).

2) Why is the langcode trimmed to 2 characters? That's a pretty bold assumption...?

3) The @todo could be clarified; had to read it twice to understand what it talks about, which is partially caused by missing context. E.g., you could do:

// Determine the language code of this translation.
// @todo Add proper methods to get and also change the langcode.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -51,21 +38,33 @@ class Field extends TypedData implements IteratorAggregate, FieldInterface {
+    // Always initialize one empty item as usually that will be needed. That
+    // way prototypes created by
+    // \Drupal\Core\TypedData\TypedDataManager::getPropertyInstance() will
+    // already have one field item ready for use after cloning.
+    $this->list[0] = $this->createItem(0);

"usually needed" by what and why?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -51,21 +38,33 @@ class Field extends TypedData implements IteratorAggregate, FieldInterface {
   public function getValue() {
...
+    if (isset($this->list)) {
+      $values = array();
+      foreach ($this->list as $delta => $item) {
+        if (!$item->isEmpty()) {
+          $values[$delta] = $item->getValue();
+        }
+        else {
+          $values[$delta] = NULL;
+        }
+      }
+      return $values;
     }
...
   }

According to this revised implementation, getValue() will return NULL if an item is empty, so why do we check for isEmpty() first?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -75,19 +74,18 @@ public function getValue() {
   public function setValue($values) {
...
+    if (!isset($values) || $values === array()) {
+      $this->list = $values;
+    }

Is there any reason for why we allow $this->list to be NULL at any time?

if (empty($values)) {
  $this->list = array();
}

would ensure that $this->list is always an array, which in turn would simplify a lot of the other methods that currently need to account for a potential NULL value - even though their behavior on NULL is identical to the behavior of an empty array.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -123,10 +110,12 @@ public function unsetValue() {
+    if (isset($this->list)) {
+      foreach ($this->list() as $item) {

Some code is accessing $this->list directly, while some other code is using the $this->list() method - the majority of code accesses the property directly instead of using the method, so I think we want to change the instances that call into the method.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -293,29 +262,27 @@ public function __unset($property_name) {
+      foreach ($this->list as $item) {
...
+      foreach ($this->list as $delta => $property) {
+        $this->list[$delta] = clone $property;

Most foreach loops call the list values $item, while __clone() calls it $property.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -70,42 +85,126 @@ public function createInstance($plugin_id, array $configuration) {
+  public function getPropertyInstance(ContextAwareInterface $object, $property_name, $value = NULL) {
+    $key = $object->getRoot()->getType() . ':' . $object->getPropertyPath() . '.';
+    // If we are creating list items, we always use 0 in the key as all list
+    // items look the same.
+    $key .= is_numeric($property_name) ? 0 : $property_name;

Wouldn't a star ('*') be more appropriate? 0 will prevent an individual list item to have a diverging data type, no?

E.g., if someone were to write a entity_reference_ng module in contrib that allows to reference multiple different entity types, then we'd have a list of varying data types, or am I mistaken?

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
@@ -48,30 +48,38 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
   protected function mapFromStorageRecords(array $records, $load_revision = FALSE) {
...
+      foreach ($record as $name => $value) {
+        $values[$name][LANGUAGE_DEFAULT][0]['value'] = $value;
+      }

This loop and value assignment seems to diverge from the regular entity storage controllers? (i.e., this is the same controller method I mentioned at the beginning, but this one does not omit [0]['value'])

Hm. Seems like tests should've barfed about #47, so we should add test coverage IMO. Could we open a follow-up for that patch so we can mark this one fixed?

Ok, fair enough.

Committed and pushed #47 to 8.x.

I've another small follow-up to propose, stemming from #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget. Here is a patch. We can commit this here or as part of the other issue, as you prefer.

The problem is very simple: if you try to access $entity->original on a BC entity you get a fatal because the original entity object is treated as an array in the magic getter. The attached patch adds a check to detect this situation.

Marking "needs review" for people to pick this up.

Tiny and trivial followup : #1880922: Remnants of entity_view_prepared flag

#55 moved to #1880926: Fatal error when retrieving $entity->original on a BC entity.