EntityNG: integrate a dynamic property data table handling

entityNG-dynamic-property-data-table-handling.patch queued for re-testing.

I think better to make tests against core entities then create abstract examples. contact category, imagestyles and views should be used in tests...
Suppose some core hooks could be wrongly fired in tests.

@andypost: This is about generic support for translatable and revisionable property tables. Your examples are config entities, those have nothing to do with this, they work completely different in that regard.

Adding all these different combinations as test entities makes IMHO sense. As a next step, we can get rid of test entities in field_test.module (test_entity_*) and instead use these for the field api and similar tests which currently depend on those.

The main question imho is if we can write the tests in a way that doesn't involve so many changes. As discussed, I like the approach taken by EntityTranslationUITest, where the test code is defined in a base class and each actual entity has it's own small class that overrides it and provides a few customisations and allows for additional type specific validations. Downside is that this multiplies the amount of test classes and is slower, to do more setUp()/tearDown()'s.

I would suggest to use 'mul' instead of 'ml' as we already use 'mul' in core:

const LANGUAGE_MULTIPLE = 'mul';

and 'rev' instead of 're' ?

Mostly nit-pick review of the actual changes, haven't looked at the tests yet.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -98,6 +111,121 @@ public function create(array $values) {
+   * @param array|null $ids
+   *   An array of entity IDs, or NULL to load all entities.
+   * @param $revision_id
+   *   The ID of the revision to load, or FALSE if this query is asking for the
+   *   most current revision(s).
+   *
+   * @return SelectQuery
+   *   A SelectQuery object for loading the entity.

New code should follow the coding style, so let's add a type to revision_id @param and fix the reference to SelectQuery. That's now \Drupal\Core\Database\...

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -158,10 +286,46 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
   /**
+   * Attaches property data in all languages for translatable properties.
+   */
+  protected function attachPropertyData(&$queried_entities, $load_revision = FALSE) {

Should have @params.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -232,7 +404,7 @@ public function save(EntityInterface $entity) {
    * Saves an entity revision.
    *
-   * @param \Drupal\Core\Entity\EntityInterface $entity
+   * @param Drupal\Core\Entity\EntityInterface $entity
    *   The entity object.
    *
    * @return integer

Looks like an accidental change?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -268,6 +440,32 @@ protected function saveRevision(EntityInterface $entity) {
+   * Saves the entity property data language aware.
+   *
+   * @param Drupal\Core\Entity\EntityInterface $entity
+   *   The entity object.
+   */
+  protected function saveData(EntityInterface $entity) {

\Drupal\...

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -314,4 +512,72 @@ protected function mapToRevisionStorageRecord(EntityInterface $entity) {
+
+  /**
+   * Maps from an entity object to the storage record of the data table.
+   */
+  protected function mapToDataStorageRecord(EntityInterface $entity, $langcode) {

Same :)

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -258,10 +280,16 @@ public function getTranslationLanguages($include_default = TRUE) {
-      if (isset($this->values[$name])) {
-        $translations += $this->values[$name];
+      // @todo Figure out why we get localized non-translatable properties here
+      // and thus have to filter them!
+      $definition = $property->getDefinition();
+      if (!empty($definition['translatable'])) {
+        if (isset($this->values[$name])) {
+          $translations += $this->values[$name];
+        }
+        $translations += $this->fields[$name];
       }
-      $translations += $this->fields[$name];

I'll try to get plach to have a look at this.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/RESTTestBase.phpundefined
+++ b/core/modules/rest/lib/Drupal/rest/Tests/RESTTestBase.phpundefined
@@ -111,7 +111,10 @@ protected function httpRequest($url, $method, $body = NULL, $format = 'applicati

@@ -111,7 +111,10 @@ protected function httpRequest($url, $method, $body = NULL, $format = 'applicati
   protected function entityCreate($entity_type) {
     switch ($entity_type) {
       case 'entity_test':
-        return entity_create('entity_test', array('name' => $this->randomName(), 'user_id' => 1));
+      case 'entity_test_rev':
+      case 'entity_test_mul':
+      case 'entity_test_mulrev':
+        return entity_create($entity_type, array('name' => $this->randomName(), 'user_id' => 1));

There's a RTBC patch for the rest test that limits those tests to just node, user and entity_test. Not sure if it makes sense to test the new test entities here too but this shouldn't be necessary anymore then unless we explicitly add them: #1847200: Limit the entity types which are used for the rest tests.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiTest.phpundefined
@@ -35,40 +35,43 @@ public static function getInfo() {
-    $entity = entity_create('entity_test', array('name' => 'test', 'user_id' => NULL));
-    $entity->save();
+    // All entity variations have to have the same results.

Thinking about ways to make the diffs for these test changes not that huge.

What about adding a helper method that receives an argument for the entity type? Then we only need to change the hardcoded 'entity_test' to $entity_test. Then we just call that once for each type we want to test which should make the diff much smaller. Can you try that for a testclass or two to see the difference?

Working on this a bit.

Ok, this should be green.

Changes:
- Renamed test helpers to assert* to not have them triggered by the test. That's what caused the failures, everything below is just "delete them functions!".
- removed str_replace('_', '-') stuff, this has been removed for node types a while ago
- merged type list functions into a single one with a filter flag, should make changes/renames easier if they would happen at some point.
- Restructured entity_test_menu() a bit to use the same page callbacks for add/delete, deleted all unused function wrappers, as the goal is to get rid of them anyway.

The difference isn't that big, but it does save ~175 lines of unecessary code, mostly in .module.

About the controllers which are added here. I do see the argument of them being separate so they are separate things, but I don't think that's necessary. Might have been easy to copy/paste them, but they also need to be reviewed now and maintained later. So I'd vote for combining them as much as possible:
- I think the overridden translation controllers should go into an EntityTranslationControllerNG. They look generic and identical and can later on be merged back into the main class once everything is NG.
- The form controllers look almost identical too, is the only difference really the redirect path and message? We can make that generic and have a single form controller for all of them then.. Might even want to fill a follow-up issue to add default implementations for save and delete right in the default controller, if there isn't one already.
- storage controller I'm not sure. they look identical right now as well, but e.g. EntityTestStorageController shouldn't have a revision_id. Maybe a base implementation that has subclasses with additional properties for multiple/revisionable? It's not like the order of properties matters much, at least not for these test entities :)

I think an awesome work is going on here! The improved test coverage is impressive, although an approach more similar to the entity translation UI tests could save us some headaches, should a particular test entity need any sort of special-casing.

Here is my review: the code looks already pretty good and I performed the smallest changes myself to avoid cluttering this with minor stuff (I hope I didn't break anything).

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -98,6 +111,121 @@ public function create(array $values) {
+    if (!empty($this->dataTable)) {
+      $query->join($this->dataTable, 'data', "data.{$this->idKey} = base.{$this->idKey}");
+    }
...
+    // Add fields from the entity data_table.
+    if (!empty($this->dataTable)) {
+      // Add all fields from the {entity_data} table.
+      $entity_data_fields = drupal_map_assoc($this->entityInfo['schema_fields_sql']['data_table']);
+      // The id field is provided by entity, so remove it.
+      unset($entity_data_fields[$this->idKey]);
+
+      if (!isset($entity_revision_fields)) {
+        $entity_revision_fields = array();
+      }
+
+      // Only add fields not covered by the base or the revision.
+      $entity_data_fields = array_diff($entity_data_fields, $entity_fields, $entity_revision_fields);
+      $query->fields('data', $entity_data_fields);
+    }

Actually this is wrong, as it is duplicating the logic in ::attachPropertyData(): the join will cause multiple entries for each entity to be returned by the main load query. This is not immediately visible since the result is keyed by entity id, however dropping all this code should be safe and correct.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -158,10 +286,51 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+      if ($load_revision) {
+        // Get revision id's.
+        $revision_ids = array();
+        foreach ($queried_entities as $id => $entity) {
+          $revision_ids[] = $entity->get($this->revisionKey)->value;
+        }
+        $query->condition($this->revisionKey, $revision_ids);
+      }

I'm not totally convinced this is right. It seems to me that we might end-up including more lines than the ones we want to retrieve. Didn't test this, though.

@Berdir, can you confirm the logic here is correct?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -258,10 +280,16 @@ public function getTranslationLanguages($include_default = TRUE) {
+      // @todo Figure out why we get localized non-translatable properties here
+      // and thus have to filter them!

This is pretty weird, but we need to refactor this code anyway (see #1810370: Entity Translation API improvements), hence I think it's a good stop-gap fix.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiTest.php
@@ -32,43 +32,58 @@ public static function getInfo() {
+    $this->assertEqual($entities[0]->name->value, 'test', $entity_type . ': Created and loaded entity.');
+    $this->assertEqual($entities[1]->name->value, 'test', $entity_type . ': Created and loaded entity.');

Here and elsewhere we need to use format_string().

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -2,11 +2,46 @@
+function _entity_test_entity_types($filter = NULL) {

This is being used throughout many many core tests, hence should not be marked as private. I'd suggest to remove the leading underscore from the function name.

Aside from the things above I think we should really introduce some intermediate classes all the test entity controllers should inherit from to factor-up the common code. Basically I agree with everything @Berdir is saying in #30 :)

[...] if there isn't one already.

http://drupal.org/project/issues/search/drupal?status[]=Open&version[]=8.x&issue_tags=Entity+forms ;)

This is for the bot, but the status is actually needs work per #32.

The improved test coverage is impressive, although an approach more similar to the entity translation UI tests could save us some headaches, should a particular test entity need any sort of special-casing.

That was exactly my initial feedback when das-peter started on this as well. However, there are two important arguments for not doing this:

- In the Translation UI tests, you are testing whith real and existing entity types from different modules which all have their special cases. And contrib modules can also use the pattern to easily add test coverage for their own translatable entity types. This is different, we are just using test entity types which were specifically created for these tests in the first place. The chance for requiring some type-specific special casing is much slimmer and if there is, we can easily ad simple if/else for that. Your tests are integration tests with those different entity types, our entity types are just helpers to test an implementation.
- Secondly, this does only slightly increase test time because it's all within one test method. Doing this your way would add 20+ (wild guess) new test classes, which is quite an overhead due to setUp() and teardown(). I think most entity tests can be converted to DrupalUnitTestBase which would eliminate most of the overhead, but still.

Will look into the other things.

This looks good to me, I performed just some minor clean-up:

• I renamed DatabaseStorageControllerNG::saveData() to DatabaseStorageControllerNG::storePropertyData() for consistency with DatabaseStorageControllerNG::attachPropertyData();
• I replaced all occurrences of the revisionable term with the revisable one, which seems to be more common.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -158,10 +200,51 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+      if ($load_revision) {
+        // Get revision id's.
+        $revision_ids = array();
+        foreach ($queried_entities as $id => $entity) {
+          $revision_ids[] = $entity->get($this->revisionKey)->value;
+        }
+        $query->condition($this->revisionKey, $revision_ids);
+      }

I'd be tempted to RTBC this, only the code above still looks suspect to me, but I didn't test it yet. Again, I'd be happy if @Berdir could confirm it's good.

Also, this is something we HAVE to do, and ASAP, not a feature.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -98,6 +111,35 @@ public function create(array $values) {
+      // @todo We should not be using a condition to specify whether conditions
+      //   apply to the default language or not. We need to move this to a
+      //   separate parameter during the following API refactoring.

We just discussed the point of @todo's in code on twitter yesterday. I know this is just moved around here, but can we add (if not yet existing) and reference a followup issue here?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -158,10 +200,51 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+      if ($load_revision) {
+        // Get revision id's.
+        $revision_ids = array();
+        foreach ($queried_entities as $id => $entity) {
+          $revision_ids[] = $entity->get($this->revisionKey)->value;
+        }
+        $query->condition($this->revisionKey, $revision_ids);

I've had to read this twice as well, but I think it's ok.

Wouldn't hurt to add something to the revisions tests specifically for the mulrev entity I guess and create multiple revisions in multiple languages, if we don't do that yet.

Nitpick: id's should be ID's.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -158,10 +200,51 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+          $queried_entities[$id]->{$name}[$langcode][0]['value'] = $values[$name];

Wondering if we want to call a method explicitly here, instead of relying on magic setters, for performance. Might also a be a bit more obvious that we're not just setting public properties here but initialize the field classes.

Not sure how easily that actually is right now.

Crosspost.

We just discussed the point of @todo's in code on twitter yesterday. I know this is just moved around here, but can we add (if not yet existing) and reference a followup issue here?

I'm not 100% sure, but I think the new EntityQuery has already language parameters...

Wondering if we want to call a method explicitly here, instead of relying on magic setters, for performance. Might also a be a bit more obvious that we're not just setting public properties here but initialize the field classes.
Not sure how easily that actually is right now.

Might be worth a try.

Entity queries do have langcode parameters but we need default_langcode parameters here. I created the related follow-up as requested (#1866330: Add a parameter to specify the default langcode value when loading/querying entities) and mentioned it in the todo.

I tried to use ::set() as suggested above, but configurable fields require compatibility mode which makes things even more unreadable. I added a todo instead.

This should be ready to go IMO.

I tried to use ::set() as suggested above, but configurable fields require compatibility mode which makes things even more unreadable. I added a todo instead.

Mmh, this makes no sense: we are attaching properties not fields. Will try to investigate this a bit more.

Yes, we discussed that part yesterday too and figured that it's not easy, so I'm fine with trying to improve that in a follow-up, it's not worth holding up the patch because of this. Once we have a proper solution for this, we probably want to use it in the existing map from storage methods as well anyway.

Ok, so what are we missing? ;)

Someone bold enough to mark this RTBC, I guess :)

About the set part, I think fago is doing all sorts of crazy things to improve the performance of the mapping/object creation, yet another reason for ignoring it here.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Plugin/Core/Entity/EntityTestRev.phpundefined
@@ -60,39 +45,13 @@ class EntityTest extends EntityNG {
-  /**
    * Overrides Entity::__construct().
    */
   public function __construct(array $values, $entity_type) {
     parent::__construct($values, $entity_type);
 
     // We unset all defined properties, so magic getters apply.
-    unset($this->id);
-    unset($this->langcode);
-    unset($this->uuid);
     unset($this->revision_id);
-    unset($this->name);
-    unset($this->user_id);
-  }

We introduce the init() helper method in the base entity class, but the different test classes have not yet been updated to override that one instead of init().

If someone else provides a new patch fixing #48, I'll dare RTBC it :)

Ok, fixed that.

#50: entityNG-data-table-1833334-50.patch queued for re-testing.

Fixed the test failures. Those are new tests, which failed because they expected a revision id and default language on the entity_test entity type and also expected a given order of the properties. Changed that to use array_diff() so that the order doesn't matter.

I only performed some minor clean-ups while reviewing the latest iterations so I feel confident about RTBC-ing this.

Let's get it in.

Put on D8MI sprint to track closely.

So finally I had a look at this. Patch generally looks good + great work on the tests.

However, I'm worried as this will quite conflict with the comment-issue improvements. I've started separating the API improvements from the comment issue, so we can build from there in other reviews and get it committed faster. See and review #1869250: Various EntityNG and TypedData API improvements. Not sure which patch is better re-rolled on top of which one?

Mmh, this makes no sense: we are attaching properties not fields. Will try to investigate this a bit more.

Well, with the new entity field API everything on an entity is called a field. Just, if you look at an entity from the typed data api perspective it's some complex data having properties, which are the entity's fields. In the entity api though, we should call them "fields" only though.

This actually means we should even change the "data" table name, e.g. from entity_test_property_data to entity_test_field_data. I think we also have some property wording left in the databasestoragecontroller, thus I guess we should do that renaming in its own issue.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -158,10 +199,54 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+   * @param array $queried_entities
+   *   Associative array of entities, keyed on the entity ID.
+   * @param boolean $load_revision
+   *   (optional) TRUE if the revision should be loaded, defaults to FALSE.
+   */
+  protected function attachPropertyData(&$queried_entities, $load_revision = FALSE) {

The function signature is wrong. It does not get entities passed, but storage records which get mapped to entities later on.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -158,10 +199,54 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+          // @todo Use EntityInterface::set() here to skip magic methods and
+          //   improve readability as soon as the Field API is converted to the
+          //   Entity Field API.

Consequently this todo is also wrong, no entities at this stage.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -258,10 +280,16 @@ public function getTranslationLanguages($include_default = TRUE) {
-        $translations += $this->values[$name];
+      // @todo Figure out why we get localized non-translatable properties here
+      // and thus have to filter them!
+      $definition = $property->getDefinition();

This sounds odd. Before committing a workaround we should at least know what is causing this. Perhaps it's even a non-issue with #1869250: Various EntityNG and TypedData API improvements thanks to the new BC-mode?

Thanks - changes look good! What about the non-translatable properties todos? For which entity type and property did the problem arise?

If the API improvements goes in first I'll happily re-roll this patch, if this patch goes in first I'll happily re-roll / extend the API patch. Only thing that matters to me is progress of some sort.

Yeah, it's time to get all those issue fixed!

I don't know, but let's see what happens with the tests if we remove that part from the patch.

Interestingly, I had this fail while working on #1869250: Various EntityNG and TypedData API improvements also - and I think it contains a fix for it - this one:

diff --git a/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.php b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.php
index eac0b7f..6ad80a3 100644
--- a/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.php
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.php
@@ -292,7 +292,7 @@ protected function getEditValues($values, $langcode, $new = FALSE) {
    *   The translation object to act on.
    */
   protected function getTranslation(EntityInterface $entity, $langcode) {
-    return $entity instanceof EntityNG ? $entity->getTranslation($langcode) : $entity;
+    return $entity instanceof EntityNG ? $entity->getTranslation($langcode, FALSE) : $entity;
   }
 
   /**

Let's see whether it's enough to fix this problem or it requires the new BC also (I can't remember the details any more) -> added this hunk to the patch.

conflicts with recently committed ng-changes of #1826688: REST module: PATCH/update, re-rolled patch.

"non-translatable properties have localized values"

Yep, the question is why. As talked via IRC it could stem from the BC mode. Is the problematic langcode the entity language only? If so, we know that it would be fixed with the new BC mode.

Yes, this makes sense :)

Let's get this in and then re-roll the other issue, which isn't quite ready yet and currently failing tests.

Good catch das-peter, makes sense. Yep, let's move on with this!

#68: entityNG-data-table-1833334-68.patch queued for re-testing.

Had a look at the test failure. What the test is trying to do is unset a dynamic field, then serialize it (where it doesn't show up at all then), then PUTs it to the service which saves the entity and because it does not exist at all, it's not updated.

What's interesting is that calling debug($entity->getPropertyValues()); in EntityResource::put() seems to internally initialize the field, with an empty array as value, then the tests pass.

I'm not sure how to properly fix this, should we ensure that all properties are initialized before calling field_attach_*(), does the serializer need to export empty but defined property as an empty array/NULL, are the tests even valid?

I also tried to only set the value to NULL like this $update_entity->field_test_text->value = NULL; instead of the current unset($update_entity->field_test_text); but even that results in the field completely missing in the serialized output, so I'm wondering if the serialization is incorrect?

ok, that's the same problem I ran into with #1869250: Various EntityNG and TypedData API improvements - where I fixed it. The problem arises thanks to http://drupal.org/node/1868274#comment-6863934.

As already fixed it #1869250: Various EntityNG and TypedData API improvements, may I propose we move on with it first and get it in asap?

#1869250: Various EntityNG and TypedData API improvements is committed now, so ... back here? :)

[wrong patch]

#68: entityNG-data-table-1833334-68.patch queued for re-testing.

Working on a re-roll, hope nobody else already is :)

Uh, that was a fun one.

This might not be perfect, especially the parts in addPropertyData(), but it was the only thing that I could get past the tests (For the record, it is awesome that we have that test coverage now, helps immensly)

@fago: "The function signature is wrong. It does not get entities passed, but storage records which get mapped to entities later on.". That was/is wrong, that function converts $records to entities but keeps calling them $records. I introduced a new $entities array to make that more obvious and changed $records in that function back to $entities.

Actually, right now, I think we can change that and add the property data to $values, which should make it considerably faster than it will be with my ugly attempt. But that will have to wait until tomorrow unless someone else wants to work on it. So we should probably merge those two functions, create $values and then instantiate the entity classes.

Another fun snippet that I fixed:

     $entity->name->value = 'foo';
-    $this->assertTrue($entity->name->value, 'foo', 'Name field set.');
+    $this->assertEqual($entity->name->value, 'foo', format_string('%entity_type: Name field set.', array('%entity_type' => $entity_type)));
     // Test removing all list items by setting it to NULL.

:)

Also, one unfortunate things about the way the test classes are structured now that Simpletest always reports the line where assert() is called in the test method, so you have to search for the assertion message to find the actual line. Not sure if we should use a different method now or how that detection exactly works, will have a look later.

#80: entityNG-data-table-1833334-80.patch queued for re-testing.

Yep, the NG-adaptions have been moved into a generally re-usable NG translation controller. Not sure why this causes test-fails with that patch?

Yes, this fails.

The thing that fails is switching the source language, then the translatable field is empty. Before I look into it, maybe @fago has an idea what could be the reason?

I think I identified the problem here, not sure how to fix it though.

When creating an entity, field widgets have LANGUAGE_NONE language. After submitting the form the languages for translatable fields are switched for the submitted values to match the form language (see EntityTranslationController::submitEntityLanguage()). This happens before building the new entity. Afterwards submitted field values are extracted in field_attach_submit(). In this case each submitted translatable field value holds two languages, for instance:

<?php
$form_state['values']['field_test_text'] === array(
  // This value is supposed to delete any previous one when changing language
  // on an already created entity.
  LANGUAGE_NONE => array(),
  // This is the submitted value.
  'en' => array(0 => array('value' => 'foo')),
);
?>

In field_attach_submit() all the available languages are processed. After debugging for a while I discovered that, due to the Entity Field API internals, when setting a value for a language all languages get the same value (I guess the culprit is a wrong reference). What is happening is that the value corresponding to the submitted form language is processed as first at line 198 of field.attach.inc and both languages (LANGUAGE_NONE and 'en') are getting the submitted value, afterwards the empty LANGUAGE_NONE value is processed and both languages get an empty array. As a result the submitted value is not stored:

<?php
// Processing $langcode 'en'
$entity->field_test_text === array(
  LANGUAGE_NONE => array(0 => array('value' => 'foo')),
  'en' => array(0 => array('value' => 'foo')),
);

// Processing $langcode LANGUAGE_NONE
$entity->field_test_text === array(
  LANGUAGE_NONE => array(),
  'en' => array(),
);
?>

As a result the submitted value is not stored

hm, so it wants to change LANGUAGE_NONE only while 'en' gets changed as well. Which one is the entity language?

After debugging for a while I discovered that, due to the Entity Field API internals, when setting a value for a language all languages get the same value (I guess the culprit is a wrong reference).

Entity Field API uses LANGUAGE_DEFAULT (what equals LANGUAGE_NONE for now) to key values in default language, so the BC decorater creates a reference to that values. If then it adds LANGUAGE_NONE values at the same time I guess it accidentally writes to the reference. We could fix that by separating LANGUAGE_NONE and LANGUAGE_DEFAULT what (I tried) does not simply work but should be doable with the new BC-mode.
However, I'm wondering why a field has values in LANGUAGE_NONE + a custom language code? Does that make sense? LANGUAGE_NONE would imply to me there cannot be anything else as it is not translatable?

At the beginning of DatabaseStorageControllerNG::attachLoad() the affected entity holds NULL in delta 0 - I guess this has to do with the prototyping(?) - after calling field_attach_load($this->entityType, $bc_entities); the entity has delta 1 with the correct value attached. Could it be that NULL values should be stripped / replaced when creating bc entities?

hm, that sounds strange. It implies that field_attach_load() adds values instead of replacing values - is that really the case? If not I guess the value gets already stored like that. So probably just #91 is the bug here, as prototyped values should be array(0 => array('value' => NULL));

hm, so it wants to change LANGUAGE_NONE only while 'en' gets changed as well. Which one is the entity language?

In this example LANGUAGE_NONE is the initial entity language, while 'en' is the submitted entity language.

However, I'm wondering why a field has values in LANGUAGE_NONE + a custom language code? Does that make sense? LANGUAGE_NONE would imply to me there cannot be anything else as it is not translatable?

It is not meant to have both LANGUAGE_NONE and 'en' stored, but when the entity language is changed, as in this case, an empty translation corresponding to the previous entity language is set for all translatable fields. This way the submitted values get the new entity language while the old values are deleted. When we complete the transition to the Entity Field API we can simply remove this stuff as field values corresponding to the original language will always be LANGUAGE_DEFAULT, while the others will always have the language corresponding to the translation they belong to. But for now we need to preserve the current behavior I guess. Or we could implement EntityFormControllerNG::submitEntityLanguage() and override this logic.

Anyway, I've no idea why tests fail only with this patch applied.

In this example LANGUAGE_NONE is the initial entity language, while 'en' is the submitted entity language.

I see. Thus, it needs to copy over default values (in pre-NG times) - but is that a reason for having both values in the submitted form values?

But for now we need to preserve the current behavior I guess. Or we could implement EntityFormControllerNG::submitEntityLanguage() and override this logic.

Yeah, or we can just override it in the NG controller to skip the copying-part as it's not needed any more - that would be reasonable.

Let's see whether this fixes the failures.

Yes, I found similar results while debugging. I managed to obtain at least correctly stored values with the attached hack (the first hunk is unrelated) but we need to incorporate it into the BC decorator somehow. As of now I'm trying to find out what's causing the mismatched deltas you are reporting above. I think this is caused by calling EntityNG::getTranslation() in DatabaseStorageControllerNG::attachPropertyData() before loading configurable field values. Digging in further.

Ok, this one should pass tests. I think this solution is pretty hackish, I hope @fago can suggest something better. I am afraid we'll need to stick with 'und' field values being stored next to language-aware values for now, becuase removing them messes the BC decorator up.

I must say I'm a bit concerned about the complexity the Entity Field API currently introduces: a bug that I would properly fix in less than a hour took me one afternoon just to provide a hackish fix. I hope I just need some more experience and gain some confidence with the Typed data stuff...

#100: entityNG-data-table-1833334-100.patch queued for re-testing.

On the // @todo Hack around Entity Field BCDecorator. change I was asking myself if und is a valid language code for a translatable field anyway, or if field_available_languages() should return only "real" languages in such a case.

Not sure it was clear from #100 but #99 is not included in the patch, because when loading field values the BC decorator expects to find 'und' values, but other pieces of code expect to find values for the original language. So we need to live with both until we remove the BC layer.

AFAICS this is the current BC decorator behavior but I might be wrong, let's wait for @fago to have the last word on this.

+    $entity_langcode = $entity->language()->langcode;
+    if ($field['translatable'] && $entity_langcode != LANGUAGE_NOT_SPECIFIED && ($index = array_search(LANGUAGE_NOT_SPECIFIED, $field_langcodes)) !== FALSE) {
+      unset($field_langcodes[$index]);
+    }

So the problem is that field_available_languages() finds 'und' as valid language?

I am afraid we'll need to stick with 'und' field values being stored next to language-aware values for now, becuase removing them messes the BC decorator up.

You may not delete 'und' values as those are expected by EntityNG as LANGUAGE_DEFAULT values. The proper fix would be changing the LANGUAGE_DEFAULT constant.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -228,17 +228,23 @@ protected function attachPropertyData(array &$entities, $load_revision = FALSE)
+          if (isset($data_fields[$name]) && !empty($definition['translatable'])) {
+            $translation->{$name}->value = $values[$name];
+          }

Is it really necessary to check the data schema here? Couldn't we just check $values - everything in the table must be in $values not?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -228,17 +228,23 @@ protected function attachPropertyData(array &$entities, $load_revision = FALSE)
+          // Avoid initializing configurable fields before loading them.
+          elseif (!empty($definition['configurable'])) {
+            unset($entities[$id]->fields[$name]);
           }

This has performance implications as objects will be destroyed and have to be re-created. We should add a $entities[$row->entity_id]->{$field_name}[$row->langcode] = array() to field_sql_storage_field_storage_load() instead.

So the problem is that field_available_languages() finds 'und' as valid language?

No, while trying to make the tests pass again, both @das-peter and I realized that currently when saving a multilingual field both an 'und' value and a duplicate one with language corresponding to the entity language are stored. This is not correct as only one should be there, but we can live with this until the Entity Field API conversion is complete. Once we get there we will have only the 'und' values (or whatever the language code for LANGUAGE_DEFAULT will be).

The problem that is making tests fail is that, since we need to get the various entity translations while attaching the data table values, all the entity fields are initialized, including configurable ones, which have no value because field_attach_load() has not be called yet. When this finally happens, field values are appended to the empty one created by EntityNG::getTranslation(), so you get an extra empty item as first one, which messes all deltas up. This happens only with translations, because it's triggered by the prototyping logic in getTranslatedField().

Working on the suggested fixes.

Now, this looks good! Thanks @fago :)

Time to get this back to RTBC!

/me cannot write :(

Fixed typo.

#110: entityNG-data-table-1833334-110.patch queued for re-testing.

indeed, that's better :) So let's move on with this!

#110: entityNG-data-table-1833334-110.patch queued for re-testing.

This looks pretty crazy. Tentatively assigning to catch.

Er. And crazy not in a "drooling on yourself" way, but more in an "over my head" way. :)

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -155,9 +196,59 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+  protected function attachPropertyData(array &$entities, $load_revision = FALSE) {
+    if ($this->dataTable) {
+      $query = db_select($this->dataTable, 'data', array('fetch' => PDO::FETCH_ASSOC))
+        ->fields('data')
+        ->condition($this->idKey, array_keys($entities))
+        ->orderBy('data.' . $this->idKey);
+      if ($load_revision) {
+        // Get revision ID's.
+        $revision_ids = array();
+        foreach ($entities as $id => $entity) {
+          $revision_ids[] = $entity->get($this->revisionKey)->value;
+        }
+        $query->condition($this->revisionKey, $revision_ids);
+      }
+      $data = $query->execute();
+
+      // Fetch the field definitions to check which field is translatable.
+      $field_definition = $this->getFieldDefinitions(array());
+      $data_fields = array_flip($this->entityInfo['schema_fields_sql']['data_table']);

This looks reversed to me. If no properties are translatable, why query the data table at all?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -263,6 +361,31 @@ protected function saveRevision(EntityInterface $entity) {
   /**
+   * Stores the entity property language-aware data.
+   *
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   The entity object.
+   */
+  protected function storePropertyData(EntityInterface $entity) {
+    // Delete and insert to handle removed values.
+    db_delete($this->dataTable)
+      ->condition($this->idKey, $entity->id())
+      ->execute();
+
+    $query = db_insert($this->dataTable);
+
+    foreach ($entity->getTranslationLanguages() as $langcode => $language) {
+      $record = $this->mapToDataStorageRecord($entity, $langcode);
+      $values = (array) $record;
+      $query
+        ->fields(array_keys($values))
+        ->values($values);
+    }
+
+    $query->execute();

This needs a transaction.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -305,4 +440,81 @@ protected function mapToRevisionStorageRecord(EntityInterface $entity) {
+    foreach ($this->entityInfo['schema_fields_sql']['data_table'] as $name) {
+      $record->$name = $translation->$name->value;

When I saw the title of this patch I was somewhat hoping it was going to kill hook_schema() for entities and instead put the responsibility for that in the Entity API instead. Follow-up?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -305,4 +440,81 @@ protected function mapToRevisionStorageRecord(EntityInterface $entity) {
+
+  /**
+   * Overwrites \Drupal\Core\Entity\DatabaseStorageController::delete().
+   */
+  public function delete(array $entities) {
+    if (!$entities) {
+      // If no IDs or invalid IDs were passed, do nothing.
+      return;

When can this happen?

When I saw the title of this patch I was somewhat hoping it was going to kill hook_schema() for entities and instead put the responsibility for that in the Entity API instead. Follow-up?

Sure, we totally need this: #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

Also:

This looks reversed to me. If no properties are translatable, why query the data table at all?

Even untranslatable properties are stored in the data table, similarly to fields. See Added multilingual support to the standard entity schema.

The attached patch renames DatabaseStorageControllerNG::storePropertyData() to DatabaseStorageControllerNG::savePropertyData() for consistency with the other method names and removes:

+    if (!$entities) {
+      // If no IDs or invalid IDs were passed, do nothing.
+      return;

which looks like unnecessary code baby-sitting. If this was needed to avoid a test failure we should fix the test instead.

@catch:

This needs a transaction.

DatabaseStorageControllerNG::savePropertyData() is called from within the transaction initiated by DatabaseStorageControllerNG::save() hence it should be safe as it is. Neither DatabaseStorageControllerNG::saveRevision() opens one for the same reason.

I think this answers all @catch's concerns, so this should be RTBC again if tests pass.

Hm, I think the empty check is not baby-sitting but defensive programming ;)

Consider this:

function entity_delete_multiple($entity_type, array $ids) {
  $controller = entity_get_controller($entity_type);
  $entities = $controller->load($ids);
  $controller->delete($entities);
}

What could happen here is that, for whatever reason, the passed in ids can not be loaded and an empty array is sent to delete(). Now, this results in a PDO Exception (id IN ()) and IMHO, it shouldn't be that easy to trigger PDO Exceptions when calling API functions.

load() checks this explicitly as well. It is possible to pass both an empty array to it (which does not return any results) or FALSE (which returns all results).

But I do not care enough to not mark it RTBC, I'll let @catch decide.

And the funny thing is yesterday I even checked load() to see what we were doing there. Here is one restoring the "defensive" lines. @catch can commit #120 or this one, as he prefers.

Table not found random failure.

#122: entityNG-data-table-1833334-122.patch queued for re-testing.

Hmm. I'd normally say that calling code should check the IDs are there before doing something with them. But in the case of delete you just want to delete stuff, so if it's not there and there's nothing to delete there's not really any harm done (i.e. if another process got to it already or something).

Committed/pushed to 8.x, will need a change notice.

Added tag

There are no public changes, so I just described what entity types need to define to be able to rely on this: http://drupal.org/node/1888646

Looks good.

It has randomly mixed up the usage of the word "properties" and "fields" though, so I've updated it to use "fields" everywhere.

I added a link to the parent change notice. The rest looks good to me. Although we may want to switch to a "multilingual" terminology instead of talking about "translation" in the clean-up phase, since this is more correct.

Thanks all, this is a great step for translatable properties :) <3

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.