Make ContentEntityDatabaseStorage generate static database schemas for content entities

Beta-blocking as a part of #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

This is already looking very good, +1 for per-entity-type services. Aside from that:

1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -113,6 +145,11 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
   +    if ($base_table = $this->entityType->getBaseTable()) {
   
   @@ -120,12 +157,14 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
   +      if ($this->revisionTable) {
   

   I think we should remove tables from the entity type definition: they don't belong there, they are a detail that matters only for the storage controller. The table name logic should be hard-coded (aside from allowing temporary table names). We may want to introduce a different key to specify the entity is not stored, but I guess it wouldn't specify a storage controller then.

2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -120,12 +157,14 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
   +    // @todo Replace this with a check for the 'language' entity key when
   +    //   https://drupal.org/node/2143729 has landed.
   

   Ok, but see above.

3. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilder.php
   @@ -0,0 +1,383 @@
   +          if (FALSE !== $key = array_search($field_name, $entity_keys)) {
   

   Please no, it took me 30 seconds to parse this :)

4. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilder.php
   @@ -0,0 +1,383 @@
   +  protected function getTables(ContentEntityType $entity_type) {
   

   As I was saying above I think we should hard-code these.

5. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilderInterface.php
   @@ -0,0 +1,47 @@
   +/**
   + * Created by PhpStorm.
   
   +++ b/core/lib/Drupal/Core/Entity/SchemaStorageControllerInterface.php
   @@ -0,0 +1,22 @@
   + * Created by PhpStorm.
   

   PhpStuff :)

6. +++ b/core/lib/Drupal/Core/Entity/Schema/EntitySchemaBuilderInterface.php
   @@ -0,0 +1,47 @@
   +  public function getAllSchema(ContentEntityType $entity_type, array $field_definitions);
   +
   ...
   +  public function getTableSchema(ContentEntityType $entity_type, array $field_definitions, $table_key);
   
   +++ b/core/lib/Drupal/Core/Entity/SchemaStorageControllerInterface.php
   @@ -0,0 +1,22 @@
   +  *   A schema array for all of the entity type's tables.
   

   Since this is a generic API would it make sense to avoid the "table" word? I guess the returned array shouldn't be required to be a valid Schema API array. This would be the case only for controllers implementing the SqlStorageInterface. Maybe just implementing on getSchema() and just returning the full array?

7. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
   @@ -9,433 +9,34 @@
   -function entity_test_schema() {
   

   Recreating the schema would help making entity tests pass :)

8. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
   @@ -134,7 +134,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setSetting('max_length', 32);
   

   Why is this needed?

If it turns out that removing table keys from entity type annotation is too big of a change, probably it make sense to defer that to a follow-up.

1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -305,12 +309,16 @@ protected function attachPropertyData(array &$entities) {
   +        $data_column_names = array_flip(array_diff(array_keys($data_revision_schema['fields']), array_keys($base_schema['fields'])));
   

   Just a reminder: we should investigate whether we need an actual schema array or a lighter data-structure is enough. Maybe we can add a @todo?

2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -442,11 +450,14 @@ protected function buildQuery($ids, $revision_id = FALSE) {
   +    $base_schema = $this->schemaBuilder->getTableSchema($this->entityType, $field_definitions, 'base_table');
   

   I guess per-entity-type services is the way: we should pass this information just once. @Crell was pointing out we should not require storage controller to have a schem builder definied, though.

3. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -726,7 +737,12 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_key
   +      $serialize = !empty($definitions[$name]->getColumns()[$definitions[$name]->getMainPropertyName()]['serialize']);
   

   Should we consider a follow-up to add a serialize() method (actual name TBD) to the field item class? Regular items would just return their value while valuess needing to be serialized would get an automatic serialize() call.

4. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -662,6 +665,25 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // Install entity type tables.
   

   I don't think we should bake this logic into the module handler, it's a new dependency I don't think we *need* to introduce. What about implementing entity_modules_installed()?

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityApiTest.php
   @@ -25,16 +25,10 @@ public static function getInfo() {
   +    $this->installEntitySchema('entity_test_mul');
   +    $this->installEntitySchema('entity_test_mulrev');
   

   This is really a cool change :)

1. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilderInterface.php
   @@ -0,0 +1,41 @@
   +/**
   + * Created by PhpStorm.
   + * User: tobias
   + * Date: 24.03.14
   + * Time: 14:23
   + */
   

   Some boilerplate generation spillover.

2. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilderInterface.php
   @@ -0,0 +1,41 @@
   +  /**
   +   * Gets the full schema array for a given entity type.
   +   *
   +   * @param \Drupal\Core\Entity\ContentEntityType $entity_type
   +   *   The entity type to return the schema for.
   +   * @param \Drupal\Core\Field\FieldDefinitionInterface[] $field_definitions
   +   *   The field definitions of the entity type, keyed by field name.
   +   *
   +   * @return array
   +   *   A schema array for the entity type's tables.
   +   */
   +  public function getSchema();
   

   Params need to be included in the function signature and given default values. Objects should be identified by Interface. There's a use statement missing for \Drupal\Core\Field\FieldDefinitionInterface

3. +++ b/core/lib/Drupal/Core/Entity/SqlStorageControllerInterface.php
   @@ -0,0 +1,15 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\SchemaStorageInterface.
   + */
   +
   +namespace Drupal\Core\Entity;
   +
   +use Drupal\Core\Entity\Schema\SchemaBuilderInterface;
   +
   +/**
   + * A common interface for SQL-based storage controllers.
   + */
   +interface SqlStorageControllerInterface extends EntityStorageControllerInterface, SchemaBuilderInterface {
   +}
   

   The @file info is wrong here.

Going to do some manual testing now.

1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -8,7 +8,10 @@
   +use Drupal\Core\Entity\Schema\SchemaBuilder;
   +use Drupal\Core\Entity\Schema\SchemaBuilderInterface;
    use Drupal\Core\Language\Language;
   

   Can we rename those ContentEntitySchemaHandler(Interface)? I just realized that schema builder is the name I wanted to give to the service actually triggering (re)builds (see #1498720-48: [meta] Make the entity storage system handle changes in the entity and field schema definitions). The prefix Content is to communicate the fact we are dealing with content entities.

2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -247,6 +297,8 @@ protected function mapFromStorageRecords(array $records) {
   +      $table_mapping = $this->getTableMapping();
   
   @@ -391,6 +444,8 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
   +    $table_mapping = $this->getTableMapping();
   

   Can we move these close to the lines where we are actually using it?

3. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -657,41 +723,64 @@ protected function savePropertyData(EntityInterface $entity, $table_key = 'data_
   +        // Since this is not a multi-column field we can assume there is only one
   

   80 chars

4. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
   @@ -0,0 +1,402 @@
   +
   

   bogus blank line

5. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
   @@ -0,0 +1,402 @@
   +   * Constructs an SchemaBuilder.
   

   "a SchemaBuilder"

   I think :)

6. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
   @@ -0,0 +1,402 @@
   +      elseif ($base_schema = drupal_get_schema($tables['base_table'])) {
   

   Please let's add a comment here: every time I see drupal_get_schema() I think "WTF?!" :)

7. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
   @@ -0,0 +1,402 @@
   +        // @todo This will become unnecessary once https://drupal.org/node/2143729
   ...
   +        // $tables and $entity_keys instead of building the array manually avoids
   ...
   +          // 'default_language' on the data table and the revision data table, if
   

   80 chars

8. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
   @@ -0,0 +1,402 @@
   +        $other_definitions = array_diff_key($storable_definitions, array_flip($entity_keys) + array('langcode' => TRUE));
   +        $other_fields = array_keys($other_definitions);
   

   $other_ is a really poor variable name prefix :)
   I had to read the code multiple times to understand what was going on, what about $data_?

9. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
   @@ -0,0 +1,402 @@
   +        $table_mapping = array_fill_keys(array_keys($tables), array_values($entity_keys));
   

   I don't get what this is doing: shouldn't we use array_combine here?

10. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +          $table_mapping = array_merge_recursive($table_mapping, array_fill_keys(array_keys($tables), array('language')));
    

    Actually the base table does not hold the language column.

11. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +
    ...
    +
    
    +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilderInterface.php
    @@ -0,0 +1,41 @@
    +use Drupal\Core\Entity\ContentEntityType;
    +
    

    bogus blank line

12. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +    $tables = $this->getTables($this->entityType);
    

    bogus parameter

13. +++ b/core/lib/Drupal/Core/Entity/Schema/SchemaBuilder.php
    @@ -0,0 +1,402 @@
    +      // @todo Descriptions, unique keys, indexes and foreign keys.
    

    We need at very least indexes/unique keys before this can be committed. We probably need a spearat method like addTableIndexes() to make it overridable per-entity-type. Or maybe those can go into the processTable* methods?

14. +++ b/core/modules/entity/entity.module
    @@ -129,16 +130,55 @@ function entity_entity_bundle_delete($entity_type_id, $bundle) {
    +function entity_module_preinstall($module) {
    

    This looks very neat now :)

15. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
    @@ -9,433 +9,36 @@
    +  foreach ($entity_type_ids as $entity_type_id) {
    +    $storage_controller = $entity_manager->getStorageController($entity_type_id);
    +
         // Auto-create fields for testing.
    -    entity_create('field_config', array(
    +    $storage_controller->create(array(
           'name' => 'field_test_text',
    -      'entity_type' => $entity_type,
    +      'entity_type' => $entity_type_id,
    +      'user_id' => 1,
           'type' => 'text',
           'cardinality' => 1,
           'translatable' => FALSE,
         ))->save();
    -    entity_create('field_instance_config', array(
    -      'entity_type' => $entity_type,
    +    $storage_controller->create(array(
    +      'entity_type' => $entity_type_id,
    +      'user_id' => 1,
           'field_name' => 'field_test_text',
    -      'bundle' => $entity_type,
    +      'bundle' => $entity_type_id,
           'label' => 'Test text-field',
         ))->save();
     
    -    entity_get_form_display($entity_type, $entity_type, 'default')
    +    entity_get_form_display($entity_type_id, $entity_type_id, 'default')
    

    This change really does not look right to me: you are using the test entity storage controller to create field/instance entities...

16. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
    @@ -94,6 +95,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['field_test_text'] = FieldDefinition::create('text')
    

    This does not look right either: the definition will be provided by the Field module once we create the related field_config/field_config_instance entities.

I like 3. But let's do that in a follow-up.

Yep, it's totally follow-up material :)

Working a bit on this.

Let's see how many failures we have now...

Fixed some more tests

posting interdiff

More fixes from my and @mauzeh's branches.

@mauzeh:

Reviewing your code, a couple of notes:

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/DrupalUnitTestBaseTest.php
   @@ -19,7 +19,7 @@ class DrupalUnitTestBaseTest extends DrupalUnitTestBase {
   +  public static $modules = array('entity', 'entity_test', 'entity_install_schema_test');
   
   @@ -33,8 +33,8 @@ public static function getInfo() {
   +    $modules = array('entity', 'entity_test', 'entity_install_schema_test');
   +    $tables = array('entity_test', 'entity_install_schema_test');
   
   @@ -43,7 +43,9 @@ function testSetUp() {
   +    foreach($tables as $table){
   +      $this->assertFalse(db_table_exists($table), "'$table' database table not found.");
   +    }
   

   Do we need still need entity/entity_test modules here?

2. +++ b/core/modules/simpletest/tests/modules/entity_install_schema_test/entity_install_schema_test.install
   @@ -0,0 +1,50 @@
   +function entity_install_schema_test_schema(){
   +  $schema['entity_install_schema_test'] = array(
   ...
   +      'entity_install_schema_test_index' => array('sid', 'dummy_integer'),
   

   I think we should call the module "simpletest_test" or something. It has nothing to do with entities so the current name is a bit misleading. By calling this way we allow it to be used also for other purposes.

3. +++ b/core/modules/simpletest/tests/modules/entity_install_schema_test/entity_install_schema_test.module
   @@ -0,0 +1,25 @@
   +function entity_install_schema_test_permission() {
   

   Why do we need these permissions?

This might be green finally.
(crossing-fingers)

That was unfair, marvin :(

This refactors the whole stuff a bit: as discussed yesterday with Tobias, we are moving the getTableMapping() method on the storage, so we can properly lazy load the schema builder (which will grow bigger in the follow-ups). Interfaces have been refactored accordingy.

Sorry, wrong patch. The interdiff in #45 is good.

Quick review...

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -133,6 +220,173 @@ public function __construct(EntityTypeInterface $entity_type, Connection $databa
   +
   +        $this->getLayoutType();
   +        switch (TRUE) {
   +          // The base layout stores all the base field values in the base table.
   +          case $this->layoutType == static::LAYOUT_BASE:
   +            $table_mapping[$this->baseTable] = $this->processFields(array_merge($key_fields, array_diff($storable_fields, $key_fields)));
   +            break;
   

   I don't even...

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -582,7 +845,17 @@ public function save(EntityInterface $entity) {
   +        // Even if this is a new entity, the ID key might have been set in which
   +        // case we should not override the provided ID. An empty value for the
   +        // id is interpreted as NULL and thus overriden.
   +        if (empty($record->{$this->idKey})) {
   +          $record->{$this->idKey} = $insert_id;
   +        }
   +        $return = SAVED_NEW;
   

   There's special cases like uid 0, wondering if that could be a problem. You don't really override that, though.

3. +++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
   @@ -0,0 +1,345 @@
   +
   +    // @todo Descriptions, unique keys, indexes and foreign keys.
   +  }
   

   How are multi-field indexes going to work?

4. +++ b/core/modules/entity/entity.module
   @@ -99,6 +100,36 @@ function entity_entity_bundle_delete($entity_type_id, $bundle) {
   +function entity_modules_installed($modules) {
   

   Are you sure this is not too late? modules might already want to create entities in hook_install()?

5. +++ b/core/modules/serialization/lib/Drupal/serialization/Tests/EntitySerializationTest.php
   @@ -101,6 +95,18 @@ public function testNormalize() {
   +      ),
   +      'revision_uid' => array(
   +        array('target_id' => NULL),
   +      ),
   +      'log' => array(
   +        array('value' => NULL),
   +      ),
   

   So we add those by default now?

6. +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.install
   index 0000000..b3d9bbc
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.module
   
   +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.module
   +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.module
   @@ -0,0 +1 @@
   
   @@ -0,0 +1 @@
   +<?php
   

   No longer needed, .module files are optional.

7. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
   @@ -86,7 +86,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
          ->setDescription(t('The bundle of the test entity.'))
   -      ->setRequired(TRUE);
   +      ->setRequired(TRUE)
   +      ->setSetting('max_length', 32);
    
   

   FYI: #1709960: declare a maximum length for entity and bundle machine names adds a constant for 32.

8. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestRev.php
   @@ -60,6 +60,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['revision_uid'] = FieldDefinition::create('entity_reference')
   +      ->setLabel(t('Revision user ID'))
   +      ->setDescription(t('The user ID of the author of the current revision.'))
   +      ->setSettings(array('target_type' => 'user'))
   +      ->setQueryable(FALSE)
   +      ->setRevisionable(TRUE);
   +
   +    $fields['log'] = FieldDefinition::create('string')
   +      ->setLabel(t('Log'))
   +      ->setDescription(t('The log entry explaining the changes in this revision.'))
   +      ->setRevisionable(TRUE)
   +      ->setTranslatable(TRUE);
   

   Ah, I see, this is manually added now, I guess to test how this will work for nodes?

   Those two fields only exist on the revision tables now, do we keep that? We don't want to see those values if we don't load a revision I think, or we risk duplicating it on save/showing it by default.

   Also, log shouldn't be Translatable, not in EntityTestRev.

Sorry, just saw #48, I will address it in the next iteration.

1. I know, I know :)
2. Yep, in that case it's not a real override
3. We are going to provide sensible defaults in the process methods that can be overriden by entity-type-specific schema builders (going to work on this now).
4. Getting field definitions before installing the schema introduces a nasty bug when enabling the Locale module, which then tries to lookup its tables to translate annotations, but they are not there yet. We will introduce a new hook to let modules act when entity schema is installed.
5. I would aim to do that, but it needs its own discussion. Now it just defaults to that.
6. Cool :)
7. Ok, we'll just skip that then
8. Yep, but I think it's no longer needed with this patch
9. I (mostly) agree but I think this needs a separate discussion

Reverted some probably unneeded changes after addressing #48.

The content translation test failures are expected because we are currently disabling translatability for entity types that do have translation support, as their schema has not been converted yet. We probably need some kind of BC layer until we are able to switch table layout, which probably needs to wait for #2068325: [META] Convert entity SQL queries to the Entity Query API.

3ca5a9b DEV 2183231: Renamed ::schemaBuilder() to ::schemaHandler().
9c1527d DEV 2183231: Restored shortcut translatability.
e531b32 DEV 2183231: Refactored entity save to avoid instantiating the schema handler.
92dfff1 DEV 2183231: Fixed entity serialization test.
969becf DEV 2183231: Reintroduced BC layer to support content translation without the data table.

This might be green again.

This should fix installation.

Let's see whether this is better:

fda182d DEV 2183231: Fixed BC layer comment.
32ad26d DEV 2183231: Fixed entity save.

More fixes

This includes the previous fixes plus support for index and keys generation. We should be ready to remove the BC layer now.

Fixed unique keys generation.

Just a quick and dirty attempt to remove all entity schemas. I was able to complete astandard installation, let's how this performs.

This one should be cleaner:

f634ebb DEV 2183231: Fixed UUID unique keys.
edd5fd6 DEV 2183231: Completed removal of content entity schemas.
c0110bf DEV 2183231: Filtered out multiple base fields from base table mapping.
86b0f73 DEV 2183231: Moved additional entity tables to the related storage classes.
99120f8 DEV 2183231: Fixed some tests.
dfe9810 DEV 2183231: Fixed entity reference hack.
d2a41b9 DEV 2183231: Fixed entity query.

Still missing:

• some details in the key generation to match exactly the original schema (unsigned int for id/revision_id), int length for id/revision_id (11 vs 10)
• entity-type specific indexes (to be added in the various buildSchema() methods)
• removing the drupal_get_schema() BC layer
• documentation for the new hooks
• fixing test failures
• improving test coverage

We should be ready for some serious review though.

@tstockler:

Thanks for keeping the work going!

- As entity query now uses getTableMapping() it fails to find the 'default_langcode' field on data tables, as we currently don't expose that as it's an implementation detail of the storage controller.

I think this is not correct, we should definitely have default_langcode in the table mapping: it's true it's a table to field representation, but I think all actual table columns should be included.

- Forum module is currently broken due to the issue mentioned in #48.4: Forum module tries to save a taxonomy term in forum_install(). This no longer works. :-/

We can move the term installation to hook_entity_schema_installed() (see user.install) to fix that.

- Node module should have a separate storage controller which adds {node_access} to the schema.

There is the node access storage that should be the perfect candidate to do that, but I think this is follow-up material.

- tracker_install() queries {node}

Can we do a quick entity query conversion or it's too much work?

I would suggest to simply roll the defaults stuff back.

I think it's fine: now that stuff is properly refactored to leverage those properties the right way, removing entity key defaults and add some @todo should not be problematic.

Also #2209049: Allow field schemas to contain unique keys would allow us to remove that hardcoded unique key from the schema handler in the first place.

I don't think that's a big deal: if that gets in before this we can adjust our code, otherwise it's a just few lines that will need to be refactored.

#20 3. (follow-up needed still?)

Also, tagging for issue summary update as a lot seems to have been sorted out since this issue was created.

Let's get a section for follow-ups and blockers.

This should fix a lot of tests...

More test fixes, done for the moment.

Just a reroll before starting work

Now for reals

A couple of fixes, I hoped to be able to work on this a bit more...

e70f72c DEV 2183231: Fixed book comment test.
0953b48 DEV 2183231: Fixed revision_uid default.

1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBookTest.php
   @@ -45,6 +45,7 @@ public function testBookCommentPrint() {
          'title' => 'Book title',
          'body' => 'Book body',
   +      'status' => NODE_PUBLISHED,
        ));
        $book_node->book['bid'] = 'new';
   

   This seems like the wrong fix? It's the same problem as feed.queued had, which had no default value. Instead, we should set the default_value on the status field...

2. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
   @@ -94,6 +94,10 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
   +      // Ensure we have a valid value for the revision author.
   +      if (!isset($record->revision_uid)) {
   +        $record->revision_uid = 0;
   +      }
   

   Same here I guess?

Not many fixes again...

@tstoeckler:

It seems we need to open a follow-up to auto-generate views data from the auto-schema. I.e. that that would no longer be a nice-to-have but at least a major bug. See the @todo in the interdiff.

We have #1740492: Implement a default entity views data handler postponed on this work. I already linked it in a @todo or two in a previous patch, I think we can do the same with the @todo you added in the last one.

+++ b/core/lib/Drupal/Core/Entity/SqlStorageInterface.php
@@ -12,6 +12,80 @@
+   * The base table layout: no revision or multilingual support for base fields.
+   *
+   * @var int
+   */
+  const LAYOUT_BASE = 0;
+
+  /**
+   * The revision table layout: provides revision support for base fields.
+   *
+   * @var int
+   */
+  const LAYOUT_REVISION = 1;
+
+  /**
+   * The multilingual table layout: provides multilingual support for base
+   * fields.
+   *
+   * @var int
+   */
+  const LAYOUT_MULTILINGUAL = 2;
+
+  /**
+   * The multilingual revision table layout: provides revision and multilingual
+   * support for base fields.
+   *
+   * @var int
+   */
+  const LAYOUT_MULTILINGUAL_REVISION = 3;

I disagree with this change, sorry: those constants (and the methods below) are all details of our core storage implementation. Other SQL-based implementations might want to store data in a completely different way. Those constants/methods would make no sense in that context. I originally thought about SqlStorageInterface as a marker interface, then I realized that we would need the ::getTableMapping() method to let View generate its data automatically, but I think there's not much more we can abstract. I think it's fair to put ContentEntityDatabaseStorage in our type-hints as in those cases we are explicitly referring to our well-known core implementation.

You might be interested in looking at the reverse procedure: e.g. building an entity object from Schema API: http://drupalcode.org/project/doctrine.git/blob/48b52566638b3e31e3e0ebff...

If you have any questions i'll be glad to help :)

This can also help: http://drupalcode.org/project/doctrine.git/blob/48b52566638b3e31e3e0ebff...

There was a trouble introduced in #2225399-65: Apply formatters and widgets to Feed base fields

It looks like there has been a change in policy

from Node.php

    // If no revision author has been set explicitly, make the node owner the
    // revision author.
    if (!$this->getRevisionAuthor()) {
      $this->setRevisionAuthorId($this->getOwnerId());
    }

So anonymous user changes will be marked as owner changes

and from UserCancelTest.php

    // Confirm that user's content has been attributed to anonymous user.
    $test_node = node_load($node->id(), TRUE);
    $this->assertTrue(($test_node->getOwnerId() == 0 && $test_node->isPublished()), 'Node of the user has been attributed to anonymous user.');
    $test_node = node_revision_load($revision, TRUE);
*    $this->assertTrue(($test_node->getRevisionAuthor()->id() == 0 && $test_node->isPublished()), 'Node revision of the user has been attributed to anonymous user.');
    $test_node = node_load($revision_node->id(), TRUE);
    $this->assertTrue(($test_node->getOwnerId() != 0 && $test_node->isPublished()), "Current revision of the user's node was not attributed to anonymous user.");

The test marked with a star (line 294) is expecting attribution by anonymous user.

There is more than this going on as the compiler error is

( ! ) Fatal error: Call to a member function id() on a non-object in XXXXX/sites/drupal/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php on line 294

Awesome work! I will try to review the patch ASAP.

About #125 it might be a good idea but we should try to get feedback from at least @fago and @berdir, before going on with coding.