diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php index 16c2ca1930b203fe9fc9a4d27706a44c2736dad1..e7109e1b75d82470baf60fb834fa7e3867f3ee28 100644 --- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php @@ -128,13 +128,30 @@ protected function installedStorageSchema() { */ public function requiresEntityStorageSchemaChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) { return - $entity_type->isRevisionable() != $original->isRevisionable() || - $entity_type->isTranslatable() != $original->isTranslatable() || - $this->hasSharedTableNameChanges($entity_type, $original) || + $this->hasSharedTableStructureChange($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 @@ public function requiresEntityDataMigration(EntityTypeInterface $entity_type, En 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(); diff --git a/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php index 63da4c36d76be42a8d21f287f7149a1bfdabf6de..2d1f997a95816ed28471d44f354a4765446e662d 100644 --- a/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php @@ -518,19 +518,11 @@ public function testEntityIndexCreateWithData() { $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 - // exception. - // @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. + // Add an entity index, run the update. Ensure that the index is created + // despite having data. $this->addEntityIndex(); - try { - $this->entityDefinitionUpdateManager->applyUpdates(); - $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.'); - } + $this->entityDefinitionUpdateManager->applyUpdates(); + $this->assertTrue($this->database->schema()->indexExists('entity_test_update', 'entity_test_update__new_index'), 'Index added.'); } /** diff --git a/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php index e9b61afb1691a2c10266f4b49698ebb1ac1dc3a9..0e97d46afda28ce6dc7533fa47d9c1ac1ff583c5 100644 --- a/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php @@ -1054,15 +1054,22 @@ public function providerTestRequiresEntityDataMigration() { 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], - // Case 4: different storage class, original storage class exists, ::hasData() === TRUE. - [$updated_entity_type_definition, $original_entity_type_definition_other_existing, TRUE, TRUE], - // Case 5: different storage class, original storage class exists, ::hasData() === FALSE. - [$updated_entity_type_definition, $original_entity_type_definition_other_existing, FALSE, FALSE], + [$updated_entity_type_definition, $original_entity_type_definition_other_nonexisting, NULL, TRUE, TRUE], + // Case 4: different storage class, original storage class exists, + // ::hasData() === TRUE. + [$updated_entity_type_definition, $original_entity_type_definition_other_existing, TRUE, TRUE, TRUE], + // 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 @@ public function providerTestRequiresEntityDataMigration() { * * @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 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition, ->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 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition, $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)); }