Entity type CRUD operations must use the last installed entity type and field storage definitions

This is the sister issue of #2860654: Field storage CRUD operations must use the last installed entity type and field storage definitions :)

I was also thinking about this in the last few days and in my mind I came to the same conclusion, that EntityDefinitionUpdateManager::installFieldStorageDefinition() should call updateEntityType() at the end if any key or revision metadata key were passed in.

So a big +1 for #7 and #8, let's try to implement it and see how it looks :)

I started poking around a bit to see whether we can use the last installed field storage definitions in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeUpdate() and I quickly found out from \Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest::testEntityTypeUpdateWithoutData() that this approach is inherently incompatible with our "automatic entity updates" system, because automatic updates rely on the dynamic runtime field storage definitions, like you also observed in #4/#5.

After spending more than a day deep down in this code, the only sane conclusion that I could come up with is that we need to remove the automatic entity updates functionality (i.e. make \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::applyUpdates() a no-op) and force every update to be explicit about which fields are being added/updated/deleted, otherwise we'll be stuck with a completely broken entity update system for the rest of D8's life cycle..

In other words, the only time when we can (and need) to use the live entity type and field storage definitions is when you first install an entity type. Every other operation needs to be made through explicit entity type updates and field storage installations/updates in hook_update_N() functions.

The most recent example of why we can not rely on dynamic runtime definitions is the major brokenness of system_update_8501() and its fallout: #2951279: After 8.5.0 upgrade: Unknown column revision.revision_default

FWIW, here's what I tried before giving up and venting in #11 :)

I just had a decently long discussion about this with @amateescu: the outcome is that I'm definitely onboard with ripping off automatic entity updates from core entirely.

Automatic entity updates were initially introduced as part of my vision to allow dynamically changing entity type revision and translation support. In this light core would only be responsible to handle no-data scenarios, thus being consistent with the D7 Field API's original handling of field tables. OTOH cases requiring data migrations were meant to be handled in contrib by a dedicated module that never came to being.

In the meantime we discovered how automatic entity updates were dangerous in the context of regular DB updates, so we already decided to ban them from regular/real life workflows before D8 was even released as stable. At that point automatic entity updates were only meant to improve DX during development. We didn't foresee people relying on drush entup to fix broken updates and making things even worse, but apparently that's exactly what people are doing, because they can.

Given the criticality of potential issues arising from automatic entity updates, I think we’d be well justified to rip them off, even if this is formally an hard BC break. Mitigating factors are:

• They are not really supposed to be used in any production scenario.
• They are not really useful, unless someone is trying to build an entity_storage module implementing the original vision. Even in this case now we have the SqlContentEntityStorageSchemaConverter that is a good starting point to implement this functionality without relying on them.
• We can move them to contrib, e.g. to devel or even a more obscure devel_entity_updates module along with the drush entup command, so the logic wouldn't be lost, just moved away from core, where it's mostly harmful.

@amateescu and I agreed that as part of finally fixing entity updates we need to make sure we always rely on installed definitions, except when installing a new entity type, so we will have to fix this issue and all the related ones to finally find ourselves in a good place.

If anyone wants to take a look, here's a WIP patch with my current approach after the discussion from #13 :)

I like the patch and all the decisions that have been made.

One note - currently the live entity type is used when installing a new base field in hook_update_N while returning the base field definition from \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions() based on specific key set on the entity type. If we use the the original entity type then the field will not be returned but it is required by the storage when installing the field. This is solvable by requiring that in such cases first the entity type is updated through \Drupal\Core\Entity\EntityDefinitionUpdateManager::updateEntityType() and then the field is installed. This is something we would have to explicitly outline in the CR.

I see that @tstoeckler talked about this case where the new field can be an entity or revision metadata key, but a custom code could return the field in \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions() based on another property of the entity type.

I think that the the "add field first - then update the entity type" flow is not universally correct and therefore it would be better to use it the other way around or is there any reason why we can't?

----

I think the removed test

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -851,13 +805,6 @@ public function testSingleActionCalls() {
-    // Ensure that a non-existing field cannot be installed.
-    $storage_definition = BaseFieldDefinition::create('string')
-      ->setLabel(t('A new revisionable base field'))
-      ->setRevisionable(TRUE);
-    $this->entityDefinitionUpdateManager->installFieldStorageDefinition('bar', 'entity_test_update', 'entity_test', $storage_definition);
-    $this->assertFalse($db_schema->fieldExists('entity_test_update', 'bar'), "A non-existing field cannot be installed.");

is testing an use case where a field is being installed, which is not being returned by \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions(). Even with the decisions made here this test is still valid so let's put it back and probably describe it better :).

I think that the the "add field first - then update the entity type" flow is not universally correct and therefore it would be better to use it the other way around or is there any reason why we can't?

I haven't posted my full train of thoughts in #14 because of the late hour, but you're right, I arrived at the same conclusion that entity type updates needs to be done first, followed by field storage operations.

As for the removed test, it is no longer valid with this new paradigm of not using live definitions for entity type CRUD operations. Take the following example:

Let's say a field is added to the code base in Drupal 8.6.0, with a corresponding hook_update_8601() implementation.

Then, the field is removed from the code base in Drupal 8.6.1, again with a corresponding hook_update_8602() implementation.

A site that is updating from 8.5.x directly to 8.6.1 (where the field doesn't exist anymore), should be able to run successfully through both hook_update_8601() and hook_update_8602() (in that order), with the final result being that the field was added and then removed from the last installed definitions.

Let's say a field is added to the code base in Drupal 8.6.0, with a corresponding hook_update_8601() implementation.

Then, the field is removed from the code base in Drupal 8.6.1, again with a corresponding hook_update_8602() implementation.

Currently our storage implementation requires that a field which is being installed is also returned either by CEB::baseFieldDefinitions or by some hook. You cannot remove it from the code base until hook_update_8602 has been executed, but also you should not return it in the code base after hook_update_8602 is executed - so writing such an update requires that you add a condition to your code base that the field is to be returned only if the schema version of the module is < 8602. What do you think - how many custom and contrib developers are aware of this?

I like this example because we could add a new test update testing exactly this behavior :).

Currently our storage implementation requires that a field which is being installed is also returned either by CEB::baseFieldDefinitions or by some hook.

Can you point to the storage code where this happens? Because that would be a bug that needs to be fixed.

First to describe how I came to this conclusion:

I was debugging for problems for a colleague of mine. He had used hook_entity_field_storage_info to introduce a new BaseFieldDefinition, but didn't manually set the name and entity type on the field definition itself. Later I've found out that the EntityFieldManager is not doing this automatically for fields introduced though this hook, but only for fields introduced by the hooks entity_base_field_info and entity_bundle_field_info. When the field was installed I got exceptions from the storage because it had no name which on itself didn't cause errors but subsequent errors. In \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::createEntitySchemaIndexes() there is the loop

      foreach ($index_keys as $key => $add_method) {
        if (!empty($schema[$key])) {
          foreach ($schema[$key] as $name => $specifier) {

and here the $specifier was an array with two NULL values, which crashed the following code:

$specifier_columns = array_map(function ($item) {
              return is_string($item) ? $item : reset($item);
            }, $specifier);

because here $item was NULL instead a string or array.

The problem was solved by manually setting the name on the field definition returned by the hook hook_entity_field_storage_info.

This is how I came to the conclusion that the storage is accessing the field definitions as returned by the entity class and by the hooks.

When the field was installed I got exceptions from the storage because it had no name which on itself didn't cause errors but subsequent errors.

The problem was solved by manually setting the name on the field definition returned by the hook hook_entity_field_storage_info.

The most important parts from the story are missing :) How was the field installed? What did you do after setting the name on the field storage definition in your hook_entity_field_storage_info, just cleared the cache / reinstalled the field / nothing at all?

I've started to branch out test coverage updates needed by the solution agreed upon here, otherwise this patch would get way too big.

The first one is: #2982759: EntityUpdateToRevisionableAndPublishableTest wrongly assumes entities should be converted to revisionable and publishable in a single step

While looking into the next steps needed here, the problems that we need to overcome are tests like \Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest::testEntityTypeUpdateWithoutData(), which calls to \Drupal\Tests\system\Functional\Entity\Traits\EntityDefinitionTestTrait::updateEntityTypeToRevisionable() and then expect that calling \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::applyUpdates() will automatically create all the revisionable tables using the live (in code) field storage definitions.

The major architectural change brought by the decision to stop using live definitions in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema is that a few methods from that class need to receive more parameters. For example:

- SqlContentEntityStorageSchema::onEntityTypeCreate(EntityTypeInterface $entity_type) - creating the schema for an entity type is an operation that needs to work with a given set of entity type and field storage definitions
- SqlContentEntityStorageSchema::onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original) - similar to above, updating the schema of an entity type also needs to receive a set of (possibly) updated field storage definitions
- SqlContentEntityStorageSchema::onEntityTypeDelete(EntityTypeInterface $entity_type) - this one doesn't need any change because it should always work with the last installed entity type and field storage definitions

Changing the signature of those methods will be quite an API break since they are provided by the very generic EntityTypeListenerInterface, so I think the only realistic option that we have is to improve \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchemaConverter() and make it the "official" API for converting an entity type to (non-)revisionable and (non-)translatable, with or without data, through a set of helper methods on EntityDefinitionUpdateManager.

Those new methods could be something like updateEntityTypeToRevisionable($entity_type, $field_storage_definitions), updateEntityTypeToTranslatable($entity_type, $field_storage_definitions) and updateEntityTypeToRevisionableAndTranslatable($entity_type, $field_storage_definitions).

Are there are any other options that I missed? In any case, thoughts on the above would be very helpful :)

Edit: fixed typos.

The most important parts from the story are missing :) How was the field installed? What did you do after setting the name on the field storage definition in your hook_entity_field_storage_info, just cleared the cache / reinstalled the field / nothing at all?

I've prepared a patch for you, so that you can test with it if you want. The places where there are if statements for debugging is where you can observe that the dynamic field storage definitions are being used when a field is installed.

1. Install Drupal. It is not necessary to create content.
2. Implement hook_entity_field_storage_info without setting manually the name on the field definition.
3. Implement hook_update_N to install the field through the entity update manager.
4. Rebuild the cache.
5. Run the update.
6. Watch how in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::createSharedTableSchema the call to $column_names = $table_mapping->getColumnNames($created_field_name); evaluate to

array (
  'target_id' => NULL,
)

6. Set the name on the field storage definition manually in the implemented hook_entity_field_storage_info() and watch how in 5. the column names are properly populated and the column names evaluate to

array (
  'target_id' => 'test',
)

Now I've tested further and I think that thanks to #2960046: Allow the table mapping to be instantiated with a custom entity type definition, on which your current patch seems to be based, is fixing the problem I am talking about.
This definitely is going in the right direction. I am sorry if I've caused some confusion.

@amateescu++

Yeah, all these patches improve quite a bit of things in this area, glad your problem is one of them :)

So we're back to "feedback needed" on #23.

I personally agree with every point in #23. If we have to break the API to solve a serious drawback of the storage, then I think that it is justified. I am not sure if we can do it on short notice in 8.6.x, but 8.7.x should be fine. The only problem that might arise is if there are updates in contrib relying on the current functionality.

What about if we decide to leave the old functionality there for already installed sites and use the new one for newly installed sites? Probably the overhead of doing this and maintaining both isn't worth it?

After discussing this with @amateescu it turns out this is "semi-blocked" on #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data. If we don't do that first, we need to comment out some tests in order for the patch to be green.

Re-uploading the patch from #14 which seems to apply just fine on 8.6.x.

This should fix most of the upgrade and kernel tests. Let's see what left :)

A few more test fixes.

Edit: EntityUpdateToRevisionableAndPublishableTest will be fixed by #2982759: EntityUpdateToRevisionableAndPublishableTest wrongly assumes entities should be converted to revisionable and publishable in a single step, so it would be nice to get that in :)

#2982759: EntityUpdateToRevisionableAndPublishableTest wrongly assumes entities should be converted to revisionable and publishable in a single step was committed, triggering a retest.

@plach, note that #2982759: EntityUpdateToRevisionableAndPublishableTest wrongly assumes entities should be converted to revisionable and publishable in a single step was only committed to 8.7.x, while the patch in #35 is being tested for 8.6.x. Anyway, it's been reverted in the meantime so we still have the same number of failures.

I'm starting to open separate issues for the rest of the fails, because at least some of them are pre-existing broken tests. The first one is #2989469: MediaUpdateTest is broken.

This will fix most of the test failures. After #2989469: MediaUpdateTest is broken gets in, the remaining failures are only those that test the functionality removed by this patch, and we need to decide what we want to do about them, and we have two options:

1) if we want to fix this bug in 8.6.x, we can disable the tests and bring them back in #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data, which is a 8.7.x-only issue
2) if we decide to fix this in 8.7.x, we have to postpone this issue on #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data

In any case, the patch is ready for final reviews :) I'm not very happy with the hacks introduced by this interdiff to keep SqlContentEntityStorageSchemaConverter working, but I'm not very worried about them because they will be removed in the issue mentioned above.

Updated the IS with the outcome of my discussion with @plach from #13.

#2989469: MediaUpdateTest is broken is in, we should be down to 7 fails now :)

And NR for #39.

Overall this looks good to me, there are a few lovely clean-ups as a bonus :)

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -302,7 +341,11 @@ public function requiresFieldDataMigration(FieldStorageDefinitionInterface $stor
   -    $this->checkEntityType($entity_type);
   

   Why are we removing this?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -412,6 +430,16 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +    if (isset($storage_definition->_skip_schema_operation)) {
   

   What about defining accessor methods for this property and making it protected? So we can formally declare it as @internal. Also, I was wondering whether there is any chance that the property might be serialized and stored in the latest installed definitions. If this were the case we could get weird bugs.

3. +++ b/core/modules/field/tests/fixtures/update/field.storage.node.field_ref_autocreate_2412569.yml
   @@ -17,4 +17,4 @@ cardinality: 1
   +custom_storage: true
   

   Huh? Is this a hack to temporarily exclude these fields from schema updates/testing? If so, can we document it somehow?

4. +++ b/core/modules/field/tests/modules/field_test/field_test.entity.inc
   @@ -21,13 +21,13 @@ function field_test_entity_type_alter(array &$entity_types) {
   +    $original = $entity_definition_update_manager->getEntityType($entity_type_id);
   

   Can we rename $original to $entity_type? It would make the new code easier to follow.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -853,13 +807,6 @@ public function testSingleActionCalls() {
   -    $this->assertFalse($db_schema->fieldExists('entity_test_update', 'bar'), "A non-existing field cannot be installed.");
   

   Why are we removing this case? I don't understand why this is not supported with this patch.

I discussed this with @alexpott and @catch and we all agreed that we would be more comfortable with this change happening in 8.7.x (hopefully ASAP), to have more time to test it and possibly get all the satellite issues in as well.

Per #39 postponing this to #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data.

Re #44:

1. For no good reason :) Brought it back.

2. This was just a workaround in case we wanted to fix this without #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data. Now that we have a decision to depend on that issue, the workaround will not be needed anymore and can be removed when we rebase this patch on top of that one.

3. That's not a hack at all, it's actually the right thing to do because \Drupal\Tests\field\Functional\Update\FieldUpdateTest and \Drupal\Tests\file\Functional\Update\FileUpdateTest are not testing the values of those fields, they test field settings so drupal-8.views_entity_reference_plugins-2429191.php and drupal-8.file_formatters_update_2677990.php are not even creating the database schema for those fields.

4. Sure thing, done.

5. See #17 for why that use-case is not valid anymore with this patch.

Hey, here's the patch from #47 re-rolled on top of #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data (#29). The test failures are the same as in #39 (report).

... and here's the diff file

Now that #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data is very close, here's what's needed to finish this one.

Some tests were updated since the last patches were posted here and another one needs to use the entity_test_update entity type now.

The parent issue is in.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -595,6 +638,16 @@ public static function getTemporaryTableMappingPrefix(EntityTypeInterface $entit
+    // @todo Remove in https://www.drupal.org/node/2984782.

@@ -602,8 +655,15 @@ public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $
+    // @todo Remove in https://www.drupal.org/node/2984782.

#2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data is in so these can be removed.

Rerolled for now. I think that removing the _skip_schema_operation stuff is blocked on #3004642: Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeListenerInterface::onFieldableEntityTypeUpdate() though.

That issue is now RTBC, so we can get rid of the _skip_schema_operation cruft. Not sending this patch to the testbot because it will fail until the other patch is committed.

To make this huge patch easier to review, I've split the test-related changes from the "interesting" part of the patch :)

Also extracted the custom_storage: true into its own issue: #3031086: Configurable fields that are created only for update path tests should be defined with custom_storage = TRUE

I had another look at the review patch:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -844,7 +872,7 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
   +      $table_mapping = $this->getTableMapping($entity_type, $this->fieldStorageDefinitions);
   

   I have a feeling of deja-vu in asking this, but is it safe to rely on the field storage definitions coming from the object state while we use a passed entity type definition? Couldn't there be a mismatch?

2. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -18,10 +18,10 @@
   +      $entity_type->set('data_table', 'entity_test_update_data');
   

   Nit: can we rename this to entity_test_update_field_data for consistency?

I have a feeling of deja-vu in asking this, but is it safe to rely on the field storage definitions coming from the object state while we use a passed entity type definition? Couldn't there be a mismatch?

The problem is.. we don't really have a choice :) SqlContentEntityStorageSchema::getEntitySchema() is one of two main extension points of the storage schema, and most entity types subclass it and override that method, so we can't change its signature.

But I think it's fine because we set the proper field storage definitions in the object state whenever we need to: in onEntityTypeCreate and onFieldableEntityTypeUpdate().

can we rename this to entity_test_update_field_data for consistency?

We could, but that means making *a lot* of changes to files/code that are not related to this issue, because \Drupal\system\Tests\Entity\EntityDefinitionTestTrait::updateEntityTypeToTranslatable() is the one which controls the name of that table in every definition update test :)

I've opened a separate issue for that: #3032611: Rename the data and revision data tables of the entity_test_update entity type to be consistent with the default table naming scheme

Ah, sorry, didn't realize it was a far reaching change, +1 to follow-up.

Both "blockers" are committed, let's get a new patch!

Here we go.

Forgot to include the test changes from #60.

Final review:
(hopefully :)

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -149,6 +171,46 @@ protected function deletedFieldsRepository() {
   +    // Construct a unique cache key based on the contents of the entity type and
   +    // field storage definitions.
   +    $cache_key_parts[] = spl_object_hash($entity_type);
   +    foreach ($field_storage_definitions as $storage_definition) {
   +      $cache_key_parts[] = spl_object_hash($storage_definition);
   +    }
   +    $cache_key = hash('sha256', implode('', $cache_key_parts));
   

   According to the documentation, the hash MAY be reused when the object is destroyed, so it seems it's not guaranteed that a clone of the original object gets the same hash. Couldn't we use serialize instead? Do we even need this cache?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -209,14 +271,15 @@ protected function hasSharedTableNameChanges(EntityTypeInterface $entity_type, E
   -      $table_mapping->allowsSharedTableStorage($storage_definition) != $table_mapping->allowsSharedTableStorage($original) ||
   -      $table_mapping->requiresDedicatedTableStorage($storage_definition) != $table_mapping->requiresDedicatedTableStorage($original)
   +      $table_mapping->allowsSharedTableStorage($storage_definition) != $original_table_mapping->allowsSharedTableStorage($original) ||
   +      $table_mapping->requiresDedicatedTableStorage($storage_definition) != $original_table_mapping->requiresDedicatedTableStorage($original)
   

   Just curious: is this actually needed? It seems those methods don't rely on the internal state.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -591,6 +634,7 @@ public static function getTemporaryTableMappingPrefix(EntityTypeInterface $entit
      public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $storage_definition) {
   +    $this->fieldStorageDefinitions[$storage_definition->getName()] = $storage_definition;
        $this->performFieldSchemaOperation('create', $storage_definition);
      }
   
   @@ -598,8 +642,6 @@ public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $
      public function onFieldStorageDefinitionUpdate(FieldStorageDefinitionInterface $storage_definition, FieldStorageDefinitionInterface $original) {
   -    // Store original definitions so that switching between shared and dedicated
   -    // field table layout works.
        $this->performFieldSchemaOperation('update', $storage_definition, $original);
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -853,13 +807,6 @@ public function testSingleActionCalls() {
   -    // Ensure that a non-existing field cannot be installed.
   -    $storage_definition = BaseFieldDefinition::create('string')
   -      ->setLabel(t('A new revisionable base field'))
   -      ->setRevisionable(TRUE);
   -    $this->entityDefinitionUpdateManager->installFieldStorageDefinition('bar', 'entity_test_update', 'entity_test', $storage_definition);
   -    $this->assertFalse($db_schema->fieldExists('entity_test_update', 'bar'), "A non-existing field cannot be installed.");
   

   Shouldn't we actually check whether a live definition of the field exists before proceeding?

4. +++ b/core/modules/contact/tests/modules/contact_storage_test/contact_storage_test.install
   @@ -9,15 +9,18 @@
   +  $entity_type->setStorageClass('\Drupal\Core\Entity\Sql\SqlContentEntityStorage');
   

   ::class

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -778,52 +778,6 @@ public function testDefinitionEvents() {
   -  public function testEntityTypeSchemaUpdateAndBaseFieldCreateWithoutData() {
   ...
   -  public function testEntityTypeSchemaUpdateAndRevisionableBaseFieldCreateWithoutData() {
   

   We will need to mention we no longer support these use cases in the CR.

6. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -1216,15 +1205,30 @@ public function testRequiresEntityDataMigration($updated_entity_type_definition,
   +    $last_installed_schema_repository = $this->getMock(EntityLastInstalledSchemaRepositoryInterface::class);
   
   @@ -1396,18 +1392,33 @@ protected function setUpStorageSchema(array $expected = []) {
   +    $last_installed_schema_repository = $this->getMock(EntityLastInstalledSchemaRepositoryInterface::class);
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -129,6 +137,7 @@ protected function setUp() {
   +    $this->entityLastInstalledSchemaRepository = $this->getMock(EntityLastInstalledSchemaRepositoryInterface::class);
   

   ::createMock()

7. I was also wondering whether we should use the entity definition update manager at https://cgit.drupalcode.org/drupal/tree/core/modules/field/src/Entity/Fi... as well.

Thanks for the final review! :P

Re #69:

1. I did a very unscientific test for it by running EntityDefinitionUpdateTest and recording the duration in different scenarios:
   

   HEAD:	Time: 1.81 minutes, Memory: 6.00MB
   patch from #68:	Time: 1.83 minutes, Memory: 6.00MB
   no cache:	Time: 2.01 minutes, Memory: 6.00MB
   cache using serialize():	Time: 2.27 minutes, Memory: 6.00MB

   Which shows that the cache is somewhat useful, but if you're really worried about spl_object_hash() collisions, it's better to just remove it than using serialize(). That's what I did in this patch.

2. Right, it's not strictly needed, reverted that change.
3. Nope, see #17 for the reason :)
4. Fixed.
5. I hope the CR is clear enough..
6. Fixed.
7. We are already using the field storage definition listener in 8.7.x.

Here's the change record: https://www.drupal.org/node/3034742

While discussing this patch with @plach, he pointed that I forgot to actually make \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::applyUpdates() a no-op and deprecated. So.. here it is :)

I think I got them all.

I think that was a random fail in a javascript test.

1. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -172,33 +172,7 @@ public function getChangeSummary() {
   +    @trigger_error('EntityDefinitionUpdateManagerInterface::applyUpdates() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::getChangeList() and execute each entity type and field storage update manually instead. See https://www.drupal.org/node/3034742.', E_USER_DEPRECATED);
   

   We're missing the deprecation test if I'm not mistaken.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -313,12 +313,6 @@ protected function getSchemaFromStorageDefinition(FieldStorageDefinitionInterfac
        // If the original storage has existing entities, or it is impossible to
        // determine if that is the case, require entity data to be migrated.
        $original_storage_class = $original->getStorageClass();
   
   @@ -388,16 +382,14 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
   +    // If shared table schema changes are needed, we can't proceed.
   +    if ($this->hasSharedTableStructureChange($entity_type, $original)) {
   

   ::requiresEntityDataMigration() also checks whether the original storage class is defined, I think we should reuse that logic as well. Actually I think we should also check that the storage class didn't change as that will also imply an unsupported change.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -15,6 +15,7 @@
   +use Drupal\Tests\Core\Database\SchemaIntrospectionTestTrait;
   
   @@ -27,6 +28,7 @@
   +  use SchemaIntrospectionTestTrait;
   

   No longer needed.

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -125,25 +124,6 @@ public function testEntityTypeUpdateWithoutData() {
   -  public function testEntityTypeUpdateWithData() {
   

   Instead of removing it, can we actually repurpose this test to provide test coverage for the table structure changes exception?

Re #76:

1. In this case, there is no direct replacement for the functionality of applyUpdates() so I don't think there's anything we can test..
2. Right, fixed.
3. Fixed.
4. Sure thing, done :)

Thanks, interdiff looks great!

In this case, there is no direct replacement for the functionality of applyUpdates() so I don't think there's anything we can test..

But we can test that error is triggered, right? Otherwise we might as well have an empty method body...

Discussed with @plach and we decided to leave the additional check whether the storage class has changed to a followup.

Added a deprecation test for EntityDefinitionUpdateManager::applyUpdates().

Looks great, thanks!

Updated the API changes section from the issue summary.

I spoke with Moshe about the plan proposed in the IS: he’s +1 on removing entup from drush core, but -1 on adding it back to devel, he’d prefer a separate module, so that's the new plan to keep BC :)

Doesn't apply to 8.7 anymore

1. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManagerInterface.php
   @@ -79,6 +79,21 @@ public function needsUpdates();
   +   *     - DEFINITION_CREATED
   +   *     - DEFINITION_UPDATED
   +   *     - DEFINITION_DELETED
   

   shouldn't we add the class prefix here - these aren't global constants

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -149,6 +164,34 @@ protected function deletedFieldsRepository() {
   +  protected function getTableMapping(EntityTypeInterface $entity_type = NULL, array $storage_definitions = NULL) {
   

   any reason we can't make the first parameter required? Could possible reduce the chance of side-effects by making it explicit for callers

3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6TestBase.php
   @@ -29,6 +29,17 @@
   +    if (in_array('comment', static::$modules, TRUE)) {
   +      $this->installEntitySchema('comment');
   +    }
   +    if (in_array('taxonomy', static::$modules, TRUE)) {
   +      $this->installEntitySchema('taxonomy_term');
   +    }
   +    if (in_array('user', static::$modules, TRUE)) {
   
   +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
   @@ -15,6 +15,19 @@
   +    if (in_array('comment', static::$modules, TRUE)) {
   +      $this->installEntitySchema('comment');
   +    }
   +    if (in_array('node', static::$modules, TRUE)) {
   +      $this->installEntitySchema('node');
   +    }
   

   i'm not sure checking static is enough here, because kernel tests do the magic 'walk through all the parents and merge to form a unique set of values for modules' logic. We might need to do similar here for the sake of sub-classes in contrib/client land - especially given these are base classes designed for extension

4. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
   @@ -172,33 +172,7 @@ public function getChangeSummary() {
   +    @trigger_error('EntityDefinitionUpdateManagerInterface::applyUpdates() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::getChangeList() and execute each entity type and field storage update manually instead. See https://www.drupal.org/node/3034742.', E_USER_DEPRECATED);
   

   Normally we also do the BC thing for people, so their code keeps working. Here we just trigger the error and bail out. Is there a reason for that? If so - we should have a comment saying why.

   Also, the change record doesn't give an example of how to 'update manually instead'. Looking at the test trait, it appears to be pretty involved, so we should definitely be providing sample code for folks to follow.

for my comment above

I'm not fully up-to-date with the comments yet, went straight to the patch, so apologies for any duplication:

1. +++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManagerInterface.php
   @@ -79,6 +79,21 @@ public function needsUpdates();
   +   *   An associative array keyed by entity type id of change descriptors. Every
   

   nit: ID

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -345,44 +387,20 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
    
   -    // If a migration is required, we can't proceed.
   -    if ($this->requiresEntityDataMigration($entity_type, $original)) {
   -      throw new EntityStorageException('The SQL storage cannot change the schema for an existing entity type (' . $entity_type->id() . ') with data.');
   +    // If shared table schema changes are needed, we can't proceed.
   +    if (!class_exists($original->getStorageClass()) || $this->hasSharedTableStructureChange($entity_type, $original)) {
   +      throw new EntityStorageException("The SQL storage cannot change the schema for an existing entity type: {$entity_type->id()}.");
        }
    
   

   Is this the right error message in the exception? It looks like the logic has changed but the message has not.

3. +++ b/core/modules/block_content/tests/src/Kernel/Migrate/MigrateBlockContentTypeTest.php
   @@ -20,8 +20,8 @@ class MigrateBlockContentTypeTest extends MigrateDrupal7TestBase {
      protected function setUp() {
        parent::setUp();
   -    $this->installConfig(['block_content']);
        $this->installEntitySchema('block_content');
   +    $this->installConfig(['block_content']);
   

   This might be explained in the issue somewhere, but why is this necessary? It could probably use its own mini change record. This seems like the more disruptive thing that the patch does. The actual change itself given it makes things more robust I think is good for a minor.

Removing the release manager review tag, change record requests already made here ought to be enough I think, I can't think of special release management considerations here beyond that really.

Thank you both for the reviews!

@larowlan, re #87:

1. Indeed we should. Fixed.
2. Sure, I don't mind making the first argument explicit. Fixed.
3. Good point! Since KernelTestBase already does that magic and it updates the module handler, we should be able to query that directly instead.
4. Not having a replacement functionality for that method is exactly the point of this issue, and the reasons are explained in the IS :)

   This is the part of the CR which tells what needs to be done instead:

   Starting with 8.7.0, Drupal core no longer provides support for automatic entity updates. Whenever an entity type or field storage definition needs to be created, changed or deleted, it has to be done with an explicit update function as provided by the Update API, and using the API provided by the entity definition update manager.

   I guess you think that's not explicit enough, right? I'll add some code examples there.

@catch, re #89:

1. Good nit :D Fixed.
2. The message has changed though. The previous one was saying that we can't change the schema of an entity type with data, and the new one removed the 'with data' part, because that's the new logic.
3. Those changes are necessary in tests that install configurable fields as part of the default configuration provided by a module. The code flow is something like this:
   • a new configurable field is installed from the default configuration of a module
   • \Drupal\field\Entity\FieldStorageConfig::preSave() triggers a call to \Drupal\Core\Field\FieldStorageDefinitionListener::onFieldStorageDefinitionCreate()
   • that call eventually ends up in the storage schema handler: \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onFieldStorageDefinitionCreate()
   • the storage schema now relies on the last installed entity type definition, instead of the live (in-code) definition as before this patch
   • which, in turn, means that the entity type schema needs to be installed before any module-provided configuration

   I'll add a separate CR for this, so keeping that tag for now.

@amateescu:

Re #87.4, I was quickly discussing this in Slack with @alexpott and he suggested not to silence the deprecation error: given that we are providing no replacement for that functionality, we should be more explicit about the deprecation.

I guess you think that's not explicit enough, right? I'll add some code examples there.

Yep, code examples is what I'm asking for - so we tell people the right way to do it.

Interdiff looks good to me. Aside from #92 and the CR updates, I think this is good to go, if the testbot agrees.

Updated the CR from https://www.drupal.org/node/3034742 to include code examples and added https://www.drupal.org/node/3036689 for #89.3.

I also agree with not silencing the deprecation :)

Crediting @alexpott for #92.

Interdiff looks good to me and the CRs have plenty of information. Back to RTBC.

The message has changed though. The previous one was saying that we can't change the schema of an entity type with data, and the new one removed the 'with data' part, because that's the new logic.

OK I missed the change, but the new message just says 'can't change for a new entity type', it gives no indication of why, which the older message did. Is there a way to make it more descriptive? (leaving RTBC, just asking).

I think we should add deleting an existing entity type and field storage definition examples to https://www.drupal.org/node/3034742

@catch, I'm a bit confused by #100, so maybe it's easier if we compare the two messages next to each other, with an example entity type ID:

The SQL storage cannot change the schema for an existing entity type (block_content) with data.

The SQL storage cannot change the schema for an existing entity type: block_content.

So the only difference (aside from removing the parenthesis) is the end of the message, the "with data" part. I'm not sure why you see the new message problematic :)

@jibran: uninstalling entity types is broken (#2655162: Fatal error when updating or deleting an entity type after removing its class/definition from the codebase) and I wouldn't really advertise it in the CR. Added an example for uninstalling a field.

"The SQL storage cannot change the schema for an existing entity type" this isn't true all the time right? Sometimes it can change the schema, otherwise we'd not be able to make entity types translatable/revisionable etc., and sometimes it can't. It gives the impression that it's impossible to change the schema (now whether or not there's existing content), as opposed to specifically via this code path.

uninstalling entity types is broken

IMO #2655162: Fatal error when updating or deleting an entity type after removing its class/definition from the codebase is showing that only one usecase is broken. After committing this patch wouldn't this work if we don't remove the definition from the code?

function mymodule_update_8001() {
  $entity_update_manager = \Drupal::entityDefinitionUpdateManager();
  $entity_type = $entity_update_manager->getEntityType('my_type');
  $entity_update_manager->uninstallEntityType($entity_type);
}

If yes, then we should add this to CR.

@catch, ahh, I finally got it! Discussed a bit with @plach and he suggested the change from this interdiff.

@jibran, ok, added it.

Rerolled. HEAD moves fast these days :)

Thanks @amateescu

Maybe this is not relevant here but I'm little confused.

After fixing this bug, if I change the cardinality of an existing configurable field on staging, export my config and then import on prod; would it update the cardinality of the configurable field or do I have to write an update hook to make the same change on prod?

Your regular workflows will remain unaffected, the only actual change is that drush entup won't work anymore until we reimplement it in contrib, which should be as easy as copying the previous function body of the ::applyUpdates() method. However, as the IS states, this code will live in a module that depends on devel to make it clear that drush entup is a developer tool and should never be part of a production workflow.

Added a paragraph to the CR summarizing #109.

Rerolled.

Really nice to commit this one, I know how many issues it took to get here.

Committed fc7f649 and pushed to 8.7.x. Thanks!

Not pushed yet.

We did discuss this on the initiative call yesterday. I raised concerns with the breaking change. Whilst I understand the motivations to remove hard to maintain code especially when it is broken in some situations there is no way can possibly guarantee that no one depends on this functionality for their production site / custom code.

They are not really supposed to be used in any production scenario.

Just because someone is not supposed to doesn't mean that they won't.

We could trigger a recoverable warning if this code is used to say that it shouldn't be used on production and will be removed from Drupal 9 instead of breaking our BC promise and removing from Drupal 8.

On the call in response to my concerns @amateescu said it would be possible to go patch to a version of the patch that left the functionality in core but made the necessary changes to core to not use it.

I brought up similar concerns yesterday in a discussion with @amateescu. I would be fine with restoring that in a follow-up, I think it is fine for that to come back during the alpha phase?

I think having the code in core is dangerous, people have broken their sites using drush entup to 'fix' updates rather than restoring a backup. Having said that I'm OK with it being in core with E_WARNING or E_ERROR. I just pushed the patch though before seeing these updates, can we open a follow-up? Or if people feel strongly we could revert and recommit, but I'll mark as fixed for now.

Published the change records.

This broke two tests in JSON:API against 8.7.x:

1. \Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest
2. \Drupal\Tests\jsonapi\Functional\EntityTestTest::testGetIndividual()

See https://www.drupal.org/pift-ci-job/1222392.

Neither of the change records help me fix those failures :(

🙏 Kudos to @amateescu for helping me fix JSON:API's tests: #3038604: Fix tests following a change in Drupal 8.7.0: #2976035. It would have taken me many hours to figure out the EntityTestTest solution!

I've updated one of the change records of this issue (https://www.drupal.org/node/3036689) to specify clearly that kernel tests must install the definitions of the entity types that they're testing.

Just to day I'm happy to overridden by @catch and that the commit has occurred. Hopefully the beta update programme and contrib will surface anything serious before 8.7.0 and give us time to adjust as necessary. These things are a hard call, especially when the incumbent code is doing dangerous things.

DER 1.x and 2.x also failed after this change so I committed https://cgit.drupalcode.org/dynamic_entity_reference/commit/?id=4b25207 and https://cgit.drupalcode.org/dynamic_entity_reference/commit/?id=fd19cd0

This issue appears to have introduced a quite odd failure in kernel tests if they attempt to install config which depends on yet-to-be-installed entity schemas. I've opened #3041224: Kernel tests fail with bad error if attempt to install config before entity schemas.

@jhedstrom:

That is documented in https://www.drupal.org/node/3036689.