Issue #2371605 by plach, larowlan: SqlContentEntityStorage::countFieldData()...

Issue #2371605 by plach, larowlan: SqlContentEntityStorage::countFieldData() fatals for revision metadata fields and the UUID field

core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php

@@ -8,6 +8,7 @@
 namespace Drupal\Core\Entity\Sql;
 
 use Drupal\Component\Utility\String;
+use Drupal\Core\Entity\ContentEntityTypeInterface;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 
 /**
@@ -15,6 +16,13 @@
  */
 class DefaultTableMapping implements TableMappingInterface {
 
+  /**
+   * The entity type definition.
+   *
+   * @var \Drupal\Core\Entity\ContentEntityTypeInterface
+   */
+  protected $entityType;
+
   /**
    * The field storage definitions of this mapping.
    *
@@ -76,11 +84,14 @@ class DefaultTableMapping implements TableMappingInterface {
   /**
    * Constructs a DefaultTableMapping.
    *
+   * @param \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type
+   *   The entity type definition.
    * @param \Drupal\Core\Field\FieldStorageDefinitionInterface[] $storage_definitions
    *   A list of field storage definitions that should be available for the
    *   field columns of this table mapping.
    */
-  public function __construct(array $storage_definitions) {
+  public function __construct(ContentEntityTypeInterface $entity_type, array $storage_definitions) {
+    $this->entityType = $entity_type;
     $this->fieldStorageDefinitions = $storage_definitions;
   }
 
@@ -126,6 +137,53 @@ public function getFieldNames($table_name) {
     return array();
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getFieldTableName($field_name) {
+    $result = NULL;
+
+    if (isset($this->fieldStorageDefinitions[$field_name])) {
+      // Since a field may be stored in more than one table, we inspect tables
+      // in order of relevance: the data table if present is the main place
+      // where field data is stored, otherwise the base table is responsible for
+      // storing field data. Revision metadata is an exception as it's stored
+      // only in the revision table.
+      // @todo The table mapping itself should know about entity tables. See
+      //   https://www.drupal.org/node/2274017.
+      /** @var \Drupal\Core\Entity\Sql\SqlContentEntityStorage $storage */
+      $storage = \Drupal::entityManager()->getStorage($this->entityType->id());
+      $table_names = array(
+        $storage->getDataTable(),
+        $storage->getBaseTable(),
+        $storage->getRevisionTable(),
+      );
+
+      // Collect field columns.
+      $field_columns = array();
+      $storage_definition = $this->fieldStorageDefinitions[$field_name];
+      foreach (array_keys($storage_definition->getColumns()) as $property_name) {
+        $field_columns[] = $this->getFieldColumnName($storage_definition, $property_name);
+      }
+
+      foreach (array_filter($table_names) as $table_name) {
+        $columns = $this->getAllColumns($table_name);
+        // We assume finding one field column belonging to the mapping is enough
+        // to identify the field table.
+        if (array_intersect($columns, $field_columns)) {
+          $result = $table_name;
+          break;
+        }
+      }
+    }
+
+    if (!isset($result)) {
+      throw new SqlContentEntityStorageException(String::format('Table information not available for the "@field_name" field.', array('@field_name' => $field_name)));
+    }
+
+    return $result;
+  }
+
   /**
    * {@inheritdoc}
    */

core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php

@@ -294,7 +294,7 @@ public function getTableMapping(array $storage_definitions = NULL) {
     //   easily instantiate a new table mapping whenever needed.
     if (!isset($this->tableMapping) || $storage_definitions) {
       $definitions = $storage_definitions ?: $this->entityManager->getFieldStorageDefinitions($this->entityTypeId);
-      $table_mapping = new DefaultTableMapping($definitions);
+      $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
 
       $definitions = array_filter($definitions, function (FieldStorageDefinitionInterface $definition) use ($table_mapping) {
         return $table_mapping->allowsSharedTableStorage($definition);
@@ -1745,19 +1745,24 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
         ->distinct(TRUE);
     }
     elseif ($table_mapping->allowsSharedTableStorage($storage_definition)) {
-      $data_table = $this->dataTable ?: $this->baseTable;
-      $query = $this->database->select($data_table, 't');
-      $columns = $storage_definition->getColumns();
-      if (count($columns) > 1) {
-        $or = $query->orConditionGroup();
-        foreach ($columns as $column_name => $data) {
-          $or->isNotNull($storage_definition->getName() . '__' . $column_name);
-        }
-        $query->condition($or);
+      // Ascertain the table this field is mapped too.
+      $field_name = $storage_definition->getName();
+      try {
+        $table_name = $table_mapping->getFieldTableName($field_name);
       }
-      else {
-        $query->isNotNull($storage_definition->getName());
+      catch (SqlContentEntityStorageException $e) {
+        // This may happen when changing field storage schema, since we are not
+        // able to use a table mapping matching the passed storage definition.
+        // @todo Revisit this once we are able to instantiate the table mapping
+        //   properly. See https://www.drupal.org/node/2274017.
+        $table_name = $this->dataTable ?: $this->baseTable;
+      }
+      $query = $this->database->select($table_name, 't');
+      $or = $query->orConditionGroup();
+      foreach (array_keys($storage_definition->getColumns()) as $property_name) {
+        $or->isNotNull($table_mapping->getFieldColumnName($storage_definition, $property_name));
       }
+      $query->condition($or);
       $query
         ->fields('t', array($this->idKey))
         ->distinct(TRUE);

core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php

@@ -102,4 +102,18 @@ public function getReservedColumns();
    *   unique among all other fields.
    */
   public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $property_name);
+
+  /**
+   * Returns the table name for a given column.
+   *
+   * @param string $field_name
+   *   The name of the entity field to return the column mapping for.
+   *
+   * @return string
+   *   Table name for the given field.
+   *
+   * @throws \Drupal\Core\Entity\Sql\SqlContentEntityStorageException
+   */
+  public function getFieldTableName($field_name);
+
 }

core/modules/system/src/Tests/Entity/EntitySchemaTest.php

@@ -63,7 +63,6 @@ public function testCustomFieldCreateDelete() {
     $this->assertTrue($this->database->schema()->tableExists($table), 'Table created');
 
     // Make sure the field schema can be deleted.
-    $this->uninstallModule('entity_schema_test');
     $this->entityManager->onFieldStorageDefinitionDelete($storage_definitions['custom_base_field']);
     $this->entityManager->onFieldStorageDefinitionDelete($storage_definitions['custom_bundle_field']);
     $this->assertFalse($this->database->schema()->fieldExists($base_table, $base_column), 'Table column dropped');
@@ -131,4 +130,20 @@ protected function refreshServices() {
     $this->database = $this->container->get('database');
   }
 
+  /**
+   * Tests that modifying the UUID field for a translatable entity works.
+   */
+  public function testModifyingTranslatableColumnSchema() {
+    $this->installModule('entity_schema_test');
+    $this->updateEntityType(TRUE);
+    $fields = ['revision_log', 'uuid'];
+    foreach ($fields as $field_name) {
+      $original_definition = $this->entityManager->getBaseFieldDefinitions('entity_test')[$field_name];
+      $new_definition = clone $original_definition;
+      $new_definition->setLabel($original_definition->getLabel() . ', the other one');
+      $this->assertTrue($this->entityManager->getStorage('entity_test')
+        ->requiresFieldDataMigration($new_definition, $original_definition));
+    }
+  }
+
 }

core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module

@@ -34,6 +34,11 @@ function entity_schema_test_entity_base_field_info(EntityTypeInterface $entity_t
       ->setLabel(t('A custom base field'));
     if (\Drupal::state()->get('entity_schema_update')) {
       $definitions += EntityTestMulRev::baseFieldDefinitions($entity_type);
+      // And add a revision log.
+      $definitions['revision_log'] = BaseFieldDefinition::create('string_long')
+        ->setLabel(t('Revision log message'))
+        ->setDescription(t('The log entry explaining the changes in this revision.'))
+        ->setRevisionable(TRUE);
     }
     return $definitions;
   }

core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php

@@ -16,6 +16,26 @@
  */
 class DefaultTableMappingTest extends UnitTestCase {
 
+  /**
+   * The entity type definition.
+   *
+   * @var \Drupal\Core\Entity\ContentEntityTypeInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $entityType;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->entityType = $this->getMock('\Drupal\Core\Entity\ContentEntityTypeInterface');
+    $this->entityType
+      ->expects($this->any())
+      ->method('id')
+      ->willReturn('entity_test');
+  }
+
   /**
    * Tests DefaultTableMapping::getTableNames().
    *
@@ -24,7 +44,7 @@ class DefaultTableMappingTest extends UnitTestCase {
   public function testGetTableNames() {
     // The storage definitions are only used in getColumnNames() so we do not
     // need to provide any here.
-    $table_mapping = new DefaultTableMapping([]);
+    $table_mapping = new DefaultTableMapping($this->entityType, []);
     $this->assertSame([], $table_mapping->getTableNames());
 
     $table_mapping->setFieldNames('foo', []);
@@ -63,7 +83,7 @@ public function testGetAllColumns() {
       'target_revision_id',
     ]);
 
-    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
     $expected = [];
     $this->assertSame($expected, $table_mapping->getAllColumns('test'));
 
@@ -161,7 +181,7 @@ public function testGetAllColumns() {
   public function testGetFieldNames() {
     // The storage definitions are only used in getColumnNames() so we do not
     // need to provide any here.
-    $table_mapping = new DefaultTableMapping([]);
+    $table_mapping = new DefaultTableMapping($this->entityType, []);
 
     // Test that requesting the list of field names for a table for which no
     // fields have been added does not fail.
@@ -190,17 +210,17 @@ public function testGetFieldNames() {
    */
   public function testGetColumnNames() {
     $definitions['test'] = $this->setUpDefinition('test', []);
-    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
     $expected = [];
     $this->assertSame($expected, $table_mapping->getColumnNames('test'));
 
     $definitions['test'] = $this->setUpDefinition('test', ['value']);
-    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
     $expected = ['value' => 'test'];
     $this->assertSame($expected, $table_mapping->getColumnNames('test'));
 
     $definitions['test'] = $this->setUpDefinition('test', ['value', 'format']);
-    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
     $expected = ['value' => 'test__value', 'format' => 'test__format'];
     $this->assertSame($expected, $table_mapping->getColumnNames('test'));
   }
@@ -214,7 +234,7 @@ public function testGetColumnNames() {
   public function testGetExtraColumns() {
     // The storage definitions are only used in getColumnNames() so we do not
     // need to provide any here.
-    $table_mapping = new DefaultTableMapping([]);
+    $table_mapping = new DefaultTableMapping($this->entityType, []);
 
     // Test that requesting the list of field names for a table for which no
     // fields have been added does not fail.
@@ -254,7 +274,7 @@ public function testGetExtraColumns() {
    */
   public function testGetFieldColumnName($base_field, $columns, $column, $expected) {
     $definitions['test'] = $this->setUpDefinition('test', $columns, $base_field);
-    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
     $result = $table_mapping->getFieldColumnName($definitions['test'], $column);
     $this->assertEquals($expected, $result);
   }
@@ -285,7 +305,7 @@ public function testGetFieldColumnNameInvalid($base_field, $columns, $column) {
       ->method('hasCustomStorage')
       ->willReturn(TRUE);
 
-    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $definitions);
     $table_mapping->getFieldColumnName($definitions['test'], $column);
   }
 
@@ -316,6 +336,116 @@ public function providerTestGetFieldColumnName() {
     return $data;
   }
 
+  /**
+   * Tests DefaultTableMapping::getFieldTableName().
+   *
+   * @param string[] $table_names
+   *   An associative array of table names that should hold the field columns,
+   *   where keys can be 'base', 'data' and 'revision'.
+   * @param string $expected
+   *   The expected table name.
+   *
+   * @covers ::getFieldTableName
+   *
+   * @dataProvider providerTestGetFieldTableName
+   */
+  public function testGetFieldTableName($table_names, $expected) {
+    $field_name = 'test';
+    $columns = ['test'];
+
+    $definition = $this->setUpDefinition($field_name, $columns);
+    $definition
+      ->expects($this->any())
+      ->method('getColumns')
+      ->willReturn($columns);
+
+    $storage = $this->getMockBuilder('\Drupal\Core\Entity\Sql\SqlContentEntityStorage')
+      ->disableOriginalConstructor()
+      ->getMock();
+
+    $storage
+      ->expects($this->any())
+      ->method('getBaseTable')
+      ->willReturn(isset($table_names['base']) ? $table_names['base'] : 'base_table');
+
+    $storage
+      ->expects($this->any())
+      ->method('getDataTable')
+      ->willReturn(isset($table_names['data']) ? $table_names['data'] : NULL);
+
+    $storage
+      ->expects($this->any())
+      ->method('getRevisionTable')
+      ->willReturn(isset($table_names['revision']) ? $table_names['revision'] : NULL);
+
+    $entity_manager = $this->getMock('\Drupal\Core\Entity\EntityManagerInterface');
+    $entity_manager
+      ->expects($this->any())
+      ->method('getStorage')
+      ->willReturn($storage);
+
+    $container = $this->getMock('\Symfony\Component\DependencyInjection\ContainerInterface');
+    $container
+      ->expects($this->any())
+      ->method('get')
+      ->willReturn($entity_manager);
+
+    \Drupal::setContainer($container);
+
+    $table_mapping = new DefaultTableMapping($this->entityType, [$field_name => $definition]);
+
+    // Add the field to all the defined tables to ensure the correct one is
+    // picked.
+    foreach ($table_names as $table_name) {
+      $table_mapping->setFieldNames($table_name, [$field_name]);
+    }
+
+    $this->assertEquals($expected, $table_mapping->getFieldTableName('test'));
+  }
+
+  /**
+   * Provides test data for testGetFieldColumnName().
+   *
+   * @return array[]
+   *   A nested array where each inner array has the following values: a list of
+   *   table names and the expected table name.
+   */
+  public function providerTestGetFieldTableName() {
+    $data = [];
+
+    $data[] = [['data' => 'data_table', 'base' => 'base_table', 'revision' => 'revision_table'], 'data_table'];
+    $data[] = [['data' => 'data_table', 'revision' => 'revision_table', 'base' => 'base_table'], 'data_table'];
+    $data[] = [['base' => 'base_table', 'data' => 'data_table', 'revision' => 'revision_table'], 'data_table'];
+    $data[] = [['base' => 'base_table', 'revision' => 'revision_table', 'data' => 'data_table'], 'data_table'];
+    $data[] = [['revision' => 'revision_table', 'data' => 'data_table', 'base' => 'base_table'], 'data_table'];
+    $data[] = [['revision' => 'revision_table', 'base' => 'base_table', 'data' => 'data_table'], 'data_table'];
+
+    $data[] = [['data' => 'data_table', 'revision' => 'revision_table'], 'data_table'];
+    $data[] = [['revision' => 'revision_table', 'data' => 'data_table'], 'data_table'];
+
+    $data[] = [['base' => 'base_table', 'revision' => 'revision_table'], 'base_table'];
+    $data[] = [['revision' => 'revision_table', 'base' => 'base_table'], 'base_table'];
+
+    $data[] = [['data' => 'data_table'], 'data_table'];
+    $data[] = [['base' => 'base_table'], 'base_table'];
+    $data[] = [['revision' => 'revision_table'], 'revision_table'];
+
+    return $data;
+  }
+
+  /**
+   * Tests DefaultTableMapping::getFieldTableName() with an invalid parameter.
+   *
+   * @expectedException \Drupal\Core\Entity\Sql\SqlContentEntityStorageException
+   * @expectedExceptionMessage Table information not available for the "invalid_field_name" field.
+   *
+   * @covers ::getFieldTableName
+   */
+  public function testGetFieldTableNameInvalid() {
+    $table_mapping = new DefaultTableMapping($this->entityType, []);
+    $table_mapping->getFieldTableName('invalid_field_name');
+  }
+
   /**
    * Sets up a field storage definition for the test.
    *

core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php

@@ -388,7 +388,7 @@ public function testGetSchemaBase() {
 
     $this->setUpStorageSchema($expected);
 
-    $table_mapping = new DefaultTableMapping($this->storageDefinitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $this->storageDefinitions);
     $table_mapping->setFieldNames('entity_test', array_keys($this->storageDefinitions));
     $table_mapping->setExtraColumns('entity_test', array('default_langcode'));
 
@@ -491,7 +491,7 @@ public function testGetSchemaRevisionable() {
 
     $this->setUpStorageSchema($expected);
 
-    $table_mapping = new DefaultTableMapping($this->storageDefinitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $this->storageDefinitions);
     $table_mapping->setFieldNames('entity_test', array_keys($this->storageDefinitions));
     $table_mapping->setFieldNames('entity_test_revision', array_keys($this->storageDefinitions));
 
@@ -581,7 +581,7 @@ public function testGetSchemaTranslatable() {
 
     $this->setUpStorageSchema($expected);
 
-    $table_mapping = new DefaultTableMapping($this->storageDefinitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $this->storageDefinitions);
     $table_mapping->setFieldNames('entity_test', array_keys($this->storageDefinitions));
     $table_mapping->setFieldNames('entity_test_field_data', array_keys($this->storageDefinitions));
 
@@ -771,7 +771,7 @@ public function testGetSchemaRevisionableTranslatable() {
 
     $this->setUpStorageSchema($expected);
 
-    $table_mapping = new DefaultTableMapping($this->storageDefinitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $this->storageDefinitions);
     $table_mapping->setFieldNames('entity_test', array_keys($this->storageDefinitions));
     $table_mapping->setFieldNames('entity_test_revision', array_keys($this->storageDefinitions));
     $table_mapping->setFieldNames('entity_test_field_data', array_keys($this->storageDefinitions));
@@ -919,7 +919,7 @@ public function testDedicatedTableSchema() {
 
     $this->setUpStorageSchema($expected);
 
-    $table_mapping = new DefaultTableMapping($this->storageDefinitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $this->storageDefinitions);
     $table_mapping->setFieldNames($entity_type_id, array_keys($this->storageDefinitions));
     $table_mapping->setExtraColumns($entity_type_id, array('default_langcode'));
 
@@ -1067,7 +1067,7 @@ public function testDedicatedTableSchemaForEntityWithStringIdentifier() {
 
     $this->setUpStorageSchema($expected);
 
-    $table_mapping = new DefaultTableMapping($this->storageDefinitions);
+    $table_mapping = new DefaultTableMapping($this->entityType, $this->storageDefinitions);
     $table_mapping->setFieldNames($entity_type_id, array_keys($this->storageDefinitions));
     $table_mapping->setExtraColumns($entity_type_id, array('default_langcode'));