Issue #2340993 by Berdir: SqlContentEntityStorageSchema::requiresEntityDataMigration() returns TRUE for cases where it should return FALSE

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

@@ -128,13 +128,30 @@ class SqlContentEntityStorageSchema implements DynamicallyFieldableEntityStorage
    */
   public function requiresEntityStorageSchemaChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
     return
-      $entity_type->isRevisionable() != $original->isRevisionable() ||
+      $this->hasSharedTableStructureChange($entity_type, $original) ||
-      $entity_type->isTranslatable() != $original->isTranslatable() ||
-      $this->hasSharedTableNameChanges($entity_type, $original) ||
       // Detect changes in key or index definitions.
       $this->getEntitySchemaData($entity_type, $this->getEntitySchema($entity_type, TRUE)) != $this->loadEntitySchemaData($original);
   }
 
+  /**
+   * Detects whether there is a change in the shared table structure.
+   *
+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
+   *   The new entity type.
+   * @param \Drupal\Core\Entity\EntityTypeInterface $original
+   *   The origin entity type.
+   *
+   * @return bool
+   *   Returns TRUE if either the revisionable or translatable flag changes or
+   *   a table has been renamed.
+   */
+  protected function hasSharedTableStructureChange(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
+    return
+      $entity_type->isRevisionable() != $original->isRevisionable() ||
+      $entity_type->isTranslatable() != $original->isTranslatable() ||
+      $this->hasSharedTableNameChanges($entity_type, $original);
+  }
+
   /**
    * Detects whether any table name got renamed in an entity type update.
    *
@@ -203,6 +220,13 @@ class SqlContentEntityStorageSchema implements DynamicallyFieldableEntityStorage
     if (!class_exists($original_storage_class)) {
       return TRUE;
     }
+
+    // Data migration is not needed when only indexes changed, as they can be
+    // applied if there is data.
+    if (!$this->hasSharedTableStructureChange($entity_type, $original)) {
+      return FALSE;
+    }
+
     // Use the original entity type since the storage has not been updated.
     $original_storage = $this->entityManager->createHandlerInstance($original_storage_class, $original);
     return $original_storage->hasData();

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

@@ -518,19 +518,11 @@ class EntityDefinitionUpdateTest extends EntityUnitTestBase {
     $entity = $this->entityManager->getStorage('entity_test_update')->create(array('name' => $name));
     $entity->save();
 
-    // Add an entity index, run the update. For now, it's expected to throw an
+    // Add an entity index, run the update. Ensure that the index is created
-    // exception.
+    // despite having data.
-    // @todo Improve SqlContentEntityStorageSchema::requiresEntityDataMigration()
-    //   to return FALSE when only index changes are required, so that it can be
-    //   applied on top of existing data: https://www.drupal.org/node/2340993.
     $this->addEntityIndex();
-    try {
+    $this->entityDefinitionUpdateManager->applyUpdates();
-      $this->entityDefinitionUpdateManager->applyUpdates();
+    $this->assertTrue($this->database->schema()->indexExists('entity_test_update', 'entity_test_update__new_index'), 'Index added.');
-      $this->fail('EntityStorageException thrown when trying to apply an update that requires data migration.');
-    }
-    catch (EntityStorageException $e) {
-      $this->pass('EntityStorageException thrown when trying to apply an update that requires data migration.');
-    }
   }
 
   /**

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

@@ -1054,15 +1054,22 @@ class SqlContentEntityStorageSchemaTest extends UnitTestCase {
 
     return [
       // Case 1: same storage class, ::hasData() === TRUE.
-      [$updated_entity_type_definition, $original_entity_type_definition, TRUE, TRUE],
+      [$updated_entity_type_definition, $original_entity_type_definition, TRUE, TRUE, TRUE],
       // Case 2: same storage class, ::hasData() === FALSE.
-      [$updated_entity_type_definition, $original_entity_type_definition, FALSE, FALSE],
+      [$updated_entity_type_definition, $original_entity_type_definition, FALSE, TRUE, FALSE],
       // Case 3: different storage class, original storage class does not exist.
-      [$updated_entity_type_definition, $original_entity_type_definition_other_nonexisting, NULL, TRUE],
+      [$updated_entity_type_definition, $original_entity_type_definition_other_nonexisting, NULL, TRUE, TRUE],
-      // Case 4: different storage class, original storage class exists, ::hasData() === TRUE.
+      // Case 4: different storage class, original storage class exists,
-      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, TRUE, TRUE],
+      // ::hasData() === TRUE.
-      // Case 5: different storage class, original storage class exists, ::hasData() === FALSE.
+      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, TRUE, TRUE, TRUE],
-      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, FALSE, FALSE],
+      // Case 5: different storage class, original storage class exists,
+      // ::hasData() === FALSE.
+      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, FALSE, TRUE, FALSE],
+      // Case 6: same storage class, ::hasData() === TRUE, no structure changes.
+      [$updated_entity_type_definition, $original_entity_type_definition, TRUE, FALSE, FALSE],
+      // Case 7: different storage class, original storage class exists,
+      //::hasData() === TRUE, no structure changes.
+      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, TRUE, FALSE, FALSE],
     ];
   }
 
@@ -1071,7 +1078,7 @@ class SqlContentEntityStorageSchemaTest extends UnitTestCase {
    *
    * @dataProvider providerTestRequiresEntityDataMigration
    */
-  public function testRequiresEntityDataMigration($updated_entity_type_definition, $original_entity_type_definition, $original_storage_has_data, $migration_required) {
+  public function testRequiresEntityDataMigration($updated_entity_type_definition, $original_entity_type_definition, $original_storage_has_data, $shared_table_structure_changed, $migration_required) {
     $this->entityType = new ContentEntityType(array(
       'id' => 'entity_test',
       'entity_keys' => array('id' => 'id'),
@@ -1081,7 +1088,7 @@ class SqlContentEntityStorageSchemaTest extends UnitTestCase {
       ->disableOriginalConstructor()
       ->getMock();
 
-    $original_storage->expects($this->exactly(is_null($original_storage_has_data) ? 0 : 1))
+    $original_storage->expects($this->exactly(is_null($original_storage_has_data) || !$shared_table_structure_changed ? 0 : 1))
       ->method('hasData')
       ->willReturn($original_storage_has_data);
 
@@ -1099,9 +1106,14 @@ class SqlContentEntityStorageSchemaTest extends UnitTestCase {
 
     $this->storageSchema = $this->getMockBuilder('Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema')
       ->setConstructorArgs(array($this->entityManager, $this->entityType, $this->storage, $connection))
-      ->setMethods(array('installedStorageSchema'))
+      ->setMethods(array('installedStorageSchema', 'hasSharedTableStructureChange'))
       ->getMock();
 
+    $this->storageSchema->expects($this->any())
+      ->method('hasSharedTableStructureChange')
+      ->with($updated_entity_type_definition, $original_entity_type_definition)
+      ->willReturn($shared_table_structure_changed);
+
     $this->assertEquals($migration_required, $this->storageSchema->requiresEntityDataMigration($updated_entity_type_definition, $original_entity_type_definition));
   }