Issue #2960046 by amateescu, tstoeckler: Allow the table mapping to be...

Issue #2960046 by amateescu, tstoeckler: Allow the table mapping to be instantiated with a custom entity type definition

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

@@ -10,6 +10,7 @@
 use Drupal\Core\Database\SchemaException;
 use Drupal\Core\Entity\ContentEntityInterface;
 use Drupal\Core\Entity\ContentEntityStorageBase;
+use Drupal\Core\Entity\ContentEntityTypeInterface;
 use Drupal\Core\Entity\EntityBundleListenerInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
@@ -305,27 +306,41 @@ public function setTemporary($temporary) {
    * {@inheritdoc}
    */
   public function getTableMapping(array $storage_definitions = NULL) {
-    $table_mapping = $this->tableMapping;
+    // If a new set of field storage definitions is passed, for instance when
+    // comparing old and new storage schema, we compute the table mapping
+    // without caching.
+    if ($storage_definitions) {
+      return $this->getCustomTableMapping($this->entityType, $storage_definitions);
+    }
 
     // If we are using our internal storage definitions, which is our main use
-    // case, we can statically cache the computed table mapping. If a new set
-    // of field storage definitions is passed, for instance when comparing old
-    // and new storage schema, we compute the table mapping without caching.
-    if (!isset($this->tableMapping) || $storage_definitions) {
-      $table_mapping_class = $this->temporary ? TemporaryTableMapping::class : DefaultTableMapping::class;
-      $definitions = $storage_definitions ?: $this->entityManager->getFieldStorageDefinitions($this->entityTypeId);
-
-      /** @var \Drupal\Core\Entity\Sql\DefaultTableMapping|\Drupal\Core\Entity\Sql\TemporaryTableMapping $table_mapping */
-      $table_mapping = $table_mapping_class::create($this->entityType, $definitions);
-
-      // Cache the computed table mapping only if we are using our internal
-      // storage definitions.
-      if (!$storage_definitions) {
-        $this->tableMapping = $table_mapping;
-      }
+    // case, we can statically cache the computed table mapping.
+    if (!isset($this->tableMapping)) {
+      $storage_definitions = $this->entityManager->getFieldStorageDefinitions($this->entityTypeId);
+
+      $this->tableMapping = $this->getCustomTableMapping($this->entityType, $storage_definitions);
     }
 
-    return $table_mapping;
+    return $this->tableMapping;
+  }
+
+  /**
+   * Gets a table mapping for the specified entity type and storage definitions.
+   *
+   * @param \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type
+   *   An entity type definition.
+   * @param \Drupal\Core\Field\FieldStorageDefinitionInterface[] $storage_definitions
+   *   An array of field storage definitions to be used to compute the table
+   *   mapping.
+   *
+   * @return \Drupal\Core\Entity\Sql\TableMappingInterface
+   *   A table mapping object for the entity's tables.
+   *
+   * @internal
+   */
+  public function getCustomTableMapping(ContentEntityTypeInterface $entity_type, array $storage_definitions) {
+    $table_mapping_class = $this->temporary ? TemporaryTableMapping::class : DefaultTableMapping::class;
+    return $table_mapping_class::create($entity_type, $storage_definitions);
   }
 
   /**
@@ -1397,14 +1412,6 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    * {@inheritdoc}
    */
   public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $storage_definition) {
-    // If we are adding a field stored in a shared table we need to recompute
-    // the table mapping.
-    // @todo This does not belong here. Remove it once we are able to generate a
-    //   fresh table mapping in the schema handler. See
-    //   https://www.drupal.org/node/2274017.
-    if ($this->getTableMapping()->allowsSharedTableStorage($storage_definition)) {
-      $this->tableMapping = NULL;
-    }
     $this->wrapSchemaException(function () use ($storage_definition) {
       $this->getStorageSchema()->onFieldStorageDefinitionCreate($storage_definition);
     });
@@ -1629,16 +1636,7 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
     elseif ($table_mapping->allowsSharedTableStorage($storage_definition)) {
       // Ascertain the table this field is mapped too.
       $field_name = $storage_definition->getName();
-      try {
-        $table_name = $table_mapping->getFieldTableName($field_name);
-      }
-      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;
-      }
+      $table_name = $table_mapping->getFieldTableName($field_name);
       $query = $this->database->select($table_name, 't');
       $or = $query->orConditionGroup();
       foreach (array_keys($storage_definition->getColumns()) as $property_name) {

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

@@ -388,36 +388,21 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
   public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
     $this->checkEntityType($entity_type);
     $schema_handler = $this->database->schema();
-    $actual_definition = $this->entityManager->getDefinition($entity_type->id());
-    // @todo Instead of switching the wrapped entity type, we should be able to
-    //   instantiate a new table mapping for each entity type definition. See
-    //   https://www.drupal.org/node/2274017.
-    $this->storage->setEntityType($entity_type);
-
-    // Delete entity tables.
-    foreach ($this->getEntitySchemaTables() as $table_name) {
+
+    $field_storage_definitions = $this->entityManager->getLastInstalledFieldStorageDefinitions($entity_type->id());
+    $table_mapping = $this->storage->getCustomTableMapping($entity_type, $field_storage_definitions);
+
+    // Delete entity and field tables.
+    foreach ($table_mapping->getTableNames() as $table_name) {
       if ($schema_handler->tableExists($table_name)) {
         $schema_handler->dropTable($table_name);
       }
     }
 
-    // Delete dedicated field tables.
-    $field_storage_definitions = $this->entityManager->getLastInstalledFieldStorageDefinitions($entity_type->id());
-    $this->originalDefinitions = $field_storage_definitions;
-    $table_mapping = $this->storage->getTableMapping($field_storage_definitions);
+    // Delete the field schema data.
     foreach ($field_storage_definitions as $field_storage_definition) {
-      // If we have a field having dedicated storage we need to drop it,
-      // otherwise we just remove the related schema data.
-      if ($table_mapping->requiresDedicatedTableStorage($field_storage_definition)) {
-        $this->deleteDedicatedTableSchema($field_storage_definition);
-      }
-      elseif ($table_mapping->allowsSharedTableStorage($field_storage_definition)) {
-        $this->deleteFieldSchemaData($field_storage_definition);
-      }
+      $this->deleteFieldSchemaData($field_storage_definition);
     }
-    $this->originalDefinitions = NULL;
-
-    $this->storage->setEntityType($actual_definition);
 
     // Delete the entity schema.
     $this->deleteEntitySchemaData($entity_type);
@@ -680,13 +665,6 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
     $entity_type_id = $entity_type->id();
 
     if (!isset($this->schema[$entity_type_id]) || $reset) {
-      // Back up the storage definition and replace it with the passed one.
-      // @todo Instead of switching the wrapped entity type, we should be able
-      //   to instantiate a new table mapping for each entity type definition.
-      //   See https://www.drupal.org/node/2274017.
-      $actual_definition = $this->entityManager->getDefinition($entity_type_id);
-      $this->storage->setEntityType($entity_type);
-
       // Prepare basic information about the entity type.
       $tables = $this->getEntitySchemaTables();
 
@@ -703,7 +681,7 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
       }
 
       // We need to act only on shared entity schema tables.
-      $table_mapping = $this->storage->getTableMapping();
+      $table_mapping = $this->storage->getCustomTableMapping($entity_type, $this->fieldStorageDefinitions);
       $table_names = array_diff($table_mapping->getTableNames(), $table_mapping->getDedicatedTableNames());
       foreach ($table_names as $table_name) {
         if (!isset($schema[$table_name])) {
@@ -753,9 +731,6 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
       }
 
       $this->schema[$entity_type_id] = $schema;
-
-      // Restore the actual definition.
-      $this->storage->setEntityType($actual_definition);
     }
 
     return $this->schema[$entity_type_id];

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

@@ -370,6 +370,9 @@ public function testGetSchemaBase() {
     $this->storage->expects($this->any())
       ->method('getTableMapping')
       ->will($this->returnValue($table_mapping));
+    $this->storage->expects($this->any())
+      ->method('getCustomTableMapping')
+      ->will($this->returnValue($table_mapping));
 
     $this->assertNull(
       $this->storageSchema->onEntityTypeCreate($this->entityType)
@@ -478,6 +481,9 @@ public function testGetSchemaRevisionable() {
     $this->storage->expects($this->any())
       ->method('getTableMapping')
       ->will($this->returnValue($table_mapping));
+    $this->storage->expects($this->any())
+      ->method('getCustomTableMapping')
+      ->will($this->returnValue($table_mapping));
 
     $this->storageSchema->onEntityTypeCreate($this->entityType);
   }
@@ -586,6 +592,9 @@ public function testGetSchemaTranslatable() {
     $this->storage->expects($this->any())
       ->method('getTableMapping')
       ->will($this->returnValue($table_mapping));
+    $this->storage->expects($this->any())
+      ->method('getCustomTableMapping')
+      ->will($this->returnValue($table_mapping));
 
     $this->assertNull(
       $this->storageSchema->onEntityTypeCreate($this->entityType)
@@ -806,6 +815,9 @@ public function testGetSchemaRevisionableTranslatable() {
     $this->storage->expects($this->any())
       ->method('getTableMapping')
       ->will($this->returnValue($table_mapping));
+    $this->storage->expects($this->any())
+      ->method('getCustomTableMapping')
+      ->will($this->returnValue($table_mapping));
 
     $this->storageSchema->onEntityTypeCreate($this->entityType);
   }
@@ -1303,6 +1315,9 @@ public function testRequiresEntityStorageSchemaChanges(ContentEntityTypeInterfac
     $this->storage->expects($this->any())
       ->method('getTableMapping')
       ->will($this->returnValue($table_mapping));
+    $this->storage->expects($this->any())
+      ->method('getCustomTableMapping')
+      ->will($this->returnValue($table_mapping));
 
     // Setup storage schema.
     if ($change_schema) {
@@ -1487,6 +1502,9 @@ public function testonEntityTypeUpdateWithNewIndex() {
     $this->storage->expects($this->any())
       ->method('getTableMapping')
       ->will($this->returnValue($table_mapping));
+    $this->storage->expects($this->any())
+      ->method('getCustomTableMapping')
+      ->will($this->returnValue($table_mapping));
 
     $this->storageSchema->expects($this->any())
       ->method('loadEntitySchemaData')