Installing a new field storage definition during a fieldable entity type update is not possible

1. +++ b/core/modules/system/tests/modules/entity_test_update/src/EventSubscriber/EntitySchemaSubscriber.php
   @@ -0,0 +1,88 @@
   +  public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
   +    if (!$this->entityTypeManager->hasDefinition('entity_test_update') || $this->state->get('ignore_entity_test_update_subscriber', FALSE)) {
   +      return;
   +    }
   +    // Add a new base field when the entity type is updated.
   +    $definitions = $this->state->get('entity_test_update.additional_base_field_definitions', []);
   +    $definitions['new_base_field'] = BaseFieldDefinition::create('string')
   +      ->setName('new_base_field')
   +      ->setLabel(t('A new base field'));
   +
   

   Should there be an assertion in a test somewhere that new_base_field was installed correctly and is usable?

2. +++ b/core/modules/system/tests/modules/entity_test_update/src/EventSubscriber/EntitySchemaSubscriber.php
   @@ -0,0 +1,88 @@
   +    try {
   +      $this->entityDefinitionUpdateManager->installFieldStorageDefinition('new_base_field', 'entity_test_update', 'entity_test_update', $definitions['new_base_field']);
   +    }
   +    catch (PluginNotFoundException $e) {
   +      // The entity_test_update entity type definition has been removed, carry on.
   +    }
   

   Does this imply that ::UPDATE is also triggered when an entity type is uninstalled?

Other than that, looks good to me.

1. The update path will fail if there is an entity field definition mismatch, but yeah - we should add an assertion
2. This one is odd, as you can see, we return early if the definition doesn't exist, but in one test case, it disappears mid way through the method - @amateescu any ideas?

Patch looks great to me, I checked the issue where event dispatching was introduced and I found no rationale for the current execution order, so I guess it was just an oversight. However I think we need to perform the same change in the following methods:

onEntityTypeCreate
onEntityTypeUpdate
onFieldStorageDefinitionCreate
onFieldStorageDefinitionUpdate

Cool, with some additional test coverage this should be good to go...

Now that I think about it I'm wondering whether we should fix deletions as well

Looks great to me!

RTBC +1

I think we should add a change record for this.

Committed 15e3a5a and pushed to 8.8.x. Thanks!

Haven't backported to 8.7.x - re-open if this really needs it, but seems like it's blocking not-yet-existing-code rather than actively causing problems.

Patch in #28 does fix the DER 8.x-2.x failing HEAD which is broken ever since #25 so bumping it to major.

It seems I was too hasty with removing the cache clears from EntityDefinitionUpdateManager in the interdiff from #22, we need to bring them back because the upgrade path from #3062434: Track the workspace of a revision in a base field and convert the workspace_association entity type to a custom index.

I think that this is a good occasion to add some dedicated test coverage so that we're protected in the future.

I spoke with @amateescu today: we agreed that it would be easier for this issue to just go back to the patch I originally RTBC'd in #20, so I went ahead and reverted https://www.drupal.org/commitlog/commit/2/15e3a5a818d82d91c06eb8bbd1371c....

Revert and get a clean patch in makes sense, but my understanding is that the patch from #18 wasn't enough to fix #2904686: ViewsEntitySchemaSubscriber does not update views base table when making an entity type translatable?

Committed 2f15462 and pushed to 8.8.x. Thanks!

Leaving at RTBC for backport consideration once the commit freeze for the bugfix release is over.

I'm +1 for a backport, but have a vested interest because this blocks Workbench Moderation (one of my modules) working properly on Drupal 8.7+

@jibran:

If the DER issue is fixed now, could you remove the contrib blocker tag, please? Thanks!

This still blocks workbench_moderation, see the linked issue, so the contrib blocker tag should stay in my opinion

@plach Thanks, for asking. Surprisingly, no the fails are not gone and are still there https://www.drupal.org/pift-ci-job/1371329
Drupal\Core\Field\FieldException: Attempt to create a field field_test that does not exist on entity type entity_test.

FWIW, reverting the issue did make DER 8.x-2.x green.

What is the next step here? DER failed today as well. Only had a green run on 6th August https://www.drupal.org/pift-ci-job/1369195 when it was reverted.

I've got similar results on https://www.drupal.org/pift-ci-job/1372591 (scroll to bottom. Daily fails since 2 Aug, except 6 Aug)

Can you debug as to why the test is now failing? Don't want to revert if its just a minor change to a kernel test etc.

The test fails seem to happen if you create a field storage config and then immediately create a field config, referencing the storage by name. The following makes at least one of the kernel tests I tried in DER pass:

diff --git a/tests/src/Kernel/DynamicEntityReferenceConfigEntityTest.php b/tests/src/Kernel/DynamicEntityReferenceConfigEntityTest.php
index 3ccee3c..d7c601b 100644
--- a/tests/src/Kernel/DynamicEntityReferenceConfigEntityTest.php
+++ b/tests/src/Kernel/DynamicEntityReferenceConfigEntityTest.php
@@ -127,7 +127,7 @@ class DynamicEntityReferenceConfigEntityTest extends EntityKernelTestBase {
       ];
     }
     $this->fieldConfig = FieldConfig::create([
-      'field_name' => $this->fieldName,
+      'field_storage' => $this->fieldStorage,
       'entity_type' => $this->entityType,
       'bundle' => $this->bundle,
       'label' => 'Field test',

So it looks like we do a too little cache clearing somewhere, but can it really be that we don't have a single test in core that referenced the field storage by name like that?

Here is a functional test.

    $this->drupalLogin($this->adminUser);
    // Add a new dynamic entity reference field.
    $this->drupalGet('entity_test/structure/entity_test/fields/add-field');
    $edit = [
      'label' => 'Foobar',
      'field_name' => 'foobar',
      'new_storage_type' => 'dynamic_entity_reference',
    ];
    $this->submitForm($edit, t('Save and continue'));

It shows the error

There was a problem creating field Foobar: Attempt to create a field field_foobar that does not exist on entity type entity_test.

• I tried a revert it passes.
• I tried 8.7.x patch from #28 after the revert on 8.8.x branch, it still fails with the same error.
• I tried removing all entity test changes from the applied patch, it still fails with the same error.

So it looks like we do a too little cache clearing somewhere, but can it really be that we don't have a single test in core that referenced the field storage by name like that?

Sure, we have such tests. For example \Drupal\KernelTests\Core\Field\FieldItemTest, which also extends from EntityKernelTestBase creates a field in the setup method just by using the field name:

   /** @var \Drupal\field\Entity\FieldStorageConfig $field_storage */
    FieldStorageConfig::create([
      'field_name' => $this->fieldName,
      'type' => 'field_test',
      'entity_type' => $entity_type_id,
      'cardinality' => 1,
    ])->save();

    FieldConfig::create([
      'entity_type' => $entity_type_id,
      'field_name' => $this->fieldName,
      'bundle' => $entity_type_id,
      'label' => 'Test field',
    ])->save();

With the attached patch the DER test DynamicEntityReferenceConfigEntityTest passes.

The test passes also if we explicitly clear the cached field definitions instead of retrieving the active ones:

      \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
      $field_storage_definitions = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($this->entity_type);

So like @Berdir said

it looks like we do a too little cache clearing somewhere

The problem is caused by the following change introduced with the patch:

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php
@@ -85,10 +85,10 @@ public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $
-    $this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::CREATE, new FieldStorageDefinitionEvent($storage_definition));
-
     $this->entityLastInstalledSchemaRepository->setLastInstalledFieldStorageDefinition($storage_definition);
     $this->entityFieldManager->clearCachedFieldDefinitions();
+
+    $this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::CREATE, new FieldStorageDefinitionEvent($storage_definition));

What now happens is that we clear the cached field definitions and then fire the create event, however at this point we are still in the presave phase of the FieldStorageConfig entity and now if during the event someone retrieves the field storage definitions from the field manager then they will get cached for that entity type and as the field storage is not yet completely saved the cached definitions will not have the new field storage included. And it happens that DER has an event subscriber which retrieves the field storage definitions, but we don't have anything like this in core.

Please note that this problem might occur at every other place where the event is now being dispatched after clearing the corresponding caches.

Will the issue still be solved if we dispatch the event before clearing the caches, but after calling

this->entityLastInstalledSchemaRepository->setLastInstalledFieldStorageDefinition($storage_definition);
 

?

Also let's include a test that retrieves the field storage definitions during the event so that we ensure this is not going to break again.

Can you debug as to why the test is now failing? Don't want to revert if its just a minor change to a kernel test etc.

@larowlan, unfortunately it does not look like a minor change in a test :(

Maybe it would be a solution to move the cache clearing of the field manager from inside the field storage definition listener to \Drupal\field\Entity\FieldStorageConfig::postSave()?

For base fields we clear the caches before invoking the corresponding methods on the field storage definition listener and therefore if we move the cache clearing to the post save of the field storage config entity then we'll not have to clear the caches twice for base fields and will clear them for the field storage config entities at the right time point.

As per @amateescu suggestion following change has fixed the DER tests.

diff --git a/src/EventSubscriber/FieldStorageSubscriber.php b/src/EventSubscriber/FieldStorageSubscriber.php
index d2ba495..cc6bcda 100644
--- a/src/EventSubscriber/FieldStorageSubscriber.php
+++ b/src/EventSubscriber/FieldStorageSubscriber.php
@@ -132,7 +132,7 @@ class FieldStorageSubscriber implements EventSubscriberInterface {
     // If we know which field is being created / updated check whether it is
     // DER.
     if ($storage instanceof SqlEntityStorageInterface && !empty($der_fields[$entity_type_id])) {
-      $storage_definitions = $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id);
+      $storage_definitions = $this->entityFieldManager->getActiveFieldStorageDefinitions($entity_type_id);
       if ($field_storage_definition) {
         $storage_definitions[$field_storage_definition->getName()] = $field_storage_definition;
       }

This is a proof of BC break. And I think that the suggestions I've made should prevent the BC break.

Just like that, we have green run on DER https://www.drupal.org/pift-ci-job/1372892.

Is this bug present in DER since #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code?

It is bug since the cache clearing was moved before dispatching the event.
I don't see anything wrong to retrieve the live definitions during the event if they are either not cached or their cache is cleared properly afterwards. There might even be valid use cases for that in custom code.

Forcing sites to change code is BC break.

I've also proposed 2 different solutions on how we could prevent the BC break - the first one in #50 and the second one in #52. Any thoughts on them?

I don't understand why don't we simply prevent the BC break by introducing one of my suggestions instead of arguing?

I would like to hear the opinion of a core committer on whether they are fine with having the BC break knowing that it is possible to overcome it and the change for that is even not hard to make and in my honest opinion totally makes sense.

Also if we want to forbid using the live field storage definitions during definition CUD then we should throw an exception and ensure they are not being used, which is even more necessary because the documentation of hook_update_N() warns you to not assume something and not to not use the API.

Also that problem can occur not only during update if you have a custom subscriber and create a field through the UI, so the problem is not limited only to updates.

I think we should try and retain BC here if possible, as ideally this could be backported to 8.7 to fix #3054765: Need to add an entity update event listener to add moderation state for newly revisionable fields (disclaimer: I have a vested interest).

It looks like @amateescu is ok with doing that (comment #61)

If so, let's revert and add a test that demonstrates the issue @jibran and @dpi are seeing as well as the change from #61

Are we in agreement?

Agreed, reverted 2f15462.

Instead of clearing caches I think we should definitely try using EntityFieldManagerInterface::useCaches() and EntityTypeManagerInterface::useCaches(). This would allow us to do only a single cache clear at the very end of the process. EntityDefinitionUpdateManager is already invoking those when computing the change list, so invoking them while running actual updates would be consistent.

Disabling the cache usage is a valid option as well.

The current implemention of ::useCaches() in the EntityFieldManager doesn't influence the static cache of ::getFieldStorageDefinitions() and this will lead to the very same problem we currently have - the static cache will be filled too early and not cleared afterwards. Or maybe this is why you've written:

This would allow us to do only a single cache clear at the very end of the process.

?

P.S.
Was it an oversight or was it intentional that ::getFieldStorageDefinitions() does not check the useCaches flag and uses the static cache regardless of the flag?

> Was it an oversight or was it intentional that ::getFieldStorageDefinitions() does not check the useCaches flag and uses the static cache regardless of the flag?

It's not a use case it was designed for (changing storage definitions while cache is disabled), it does flush static caches when being enabled/disabled. not sure how often it's called during such a process, could be quite a lot?

it does flush static caches when being enabled/disabled.

Actually this happens only when being disabled. When being enabled the static caches remain intact.

@hchonov:

Or maybe this is why you've written:

This would allow us to do only a single cache clear at the very end of the process.

?

Yep, I was hoping it would be ok to statically cache those definitions, since re-invoking hooks would lead to the same result, I think, until the process is over and caches are actually cleared.

Yep, I was hoping it would be ok to statically cache those definitions, since re-invoking hooks would lead to the same result, I think, until the process is over and caches are actually cleared.

@plach, thank you for the clarification.

I am +1 on this approach then.

This was discussed in #65, #66 and #69.

I think what @plach meant was the following, which basically clears the caches before and after the event and prevents the reading from and writing to persistent caches during the event propagation, but allows for using the static caches:

$this->entityLastInstalledSchemaRepository->setLastInstalledFieldStorageDefinition($storage_definition);

$this->entityTypeManager->useCaches(FALSE);
$this->entityFieldManager->useCaches(FALSE);

$$this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::CREATE, new FieldStorageDefinitionEvent($storage_definition));

$this->entityTypeManager->useCaches(TRUE);
$this->entityFieldManager->useCaches(TRUE);

$this->entityTypeManager->clearCachedFieldDefinitions();
$this->entityFieldManager->clearCachedFieldDefinitions();

Yep, that's what I meant, but I think I'm fine with #72, as well. I have to step through the code and figure out whether there are actual benefits with doing #73, unless someone else beat me to it :)

#72 is what I've suggested myself in #50 and therefore I am fine with that approach as well :).

The only advantage I see about #73 is that it

prevents the reading from and writing to persistent caches during the event propagation

.

I spoke about this with @larowlan and we both agree that we should provide test coverage for the cache pollution issue.

This was my rationale:

1. Getting live definitions in event subscribers is not something that we should forbid because those are not only firing during DB updates: Field UI (or an hypothetical "Entity Storage" module dynamically converting entity types to non-revisionable/revisionable, non-translatable/translatable) could trigger these as well. As long as live definitions are not used to affect the schema, I think it should be ok to retrieve them freely. For instance they might be useful to compute a diff, as the code computing the change list is doing.
2. This uncovered a DER bug: since it is trying to update its own schema as a consequence of a schema update, it should retrieve active definitions to do its thing. In fact, as pointed out in #53, that was fixing the test failures. I don't think exposing a contrib bug qualifies as a BC break and if the only way for DER to keep working were to change its code, I'd be ok with that. Maybe that would be an argument not to backport the fix to 8.7.x, but that's about it. Id' still recommend DER to make that change regardless of what happens here.
3. This uncovered a core bug: we cannot have definition cache polluted by simply retrieving the live definitions in event listeners, even if that were wrong/unsupported (and it is not per #1).

@amateescu suggested to split this issue to deal with entity type and field definitions separately, but I don't think that would be acceptable: this would risk to introduce an inconsistency if the two fixes were not part of the same release. This is a very complex area and I don't think we want/can afford to introduce more technical debt. The only way to split this IMO would be to fix the cache pollution issue separately and postpone this issue on that one, but I don't think that's needed.

If @amateescu has no bandwidth to work on the test coverage, that's perfectly ok, I will try to provide a patch myself ASAP, if none beats me to it.

The test and the interdiffs look great. Thank you, @amateescu.

Only one nit:

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -847,6 +847,11 @@ public function testDefinitionEvents() {
+    $field_storage_definitions = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions('entity_test_rev');

@@ -854,11 +859,21 @@ public function testDefinitionEvents() {
+    $field_storage_definitions = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions('entity_test_rev');
...
+    $field_storage_definitions = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions('entity_test_rev');

As we now retrieve the entity field manager at 5 places in this test class, it would be better to define it as a class property and retrieve it in the setup method.

Just to follow up from the call @plach and I had last night, agree we should be fixing the cache pollution issue while we're here.

Thanks for your candour @amateescu, @plach and @hchonov and for the fast turnaround on the test-coverage @amateescu

Thank you, @amateescu.

I had one more look over it and found just one nitpick, that I hope could be fixed on commit.

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestDefinitionSubscriber.php
@@ -130,4 +240,30 @@ protected function storeEvent($event_name) {
+   *   TRUE if the last installed entity type of field storage definitions have
+   *   been updated before the was fired, FALSE otherwise.

"before the was fired" => "before the event was fired"

Committed 15c7c6b and pushed to 8.8.x. Thanks!

Leaving at RTBC for possible backport, let's let this have a few days to see if there are any contrib issues like we had before.

👌

@larowlan Any reason found to stop this from being backported to D8.7?

After speaking with @larowlan and @catch, we agreed it should be safe now to backport this to 8.7.x, so here we are :)

Please keep an eye out for potential regressions, as we have time until the next patch release to revert this.

Added release notes snippet.