Issue #3056816 by amateescu, larowlan, plach, Sam152, catch: Installing a new field storage definition during a fieldable entity type update is not possible

core/lib/Drupal/Core/Entity/EntityTypeListener.php

@@ -71,12 +71,12 @@ class EntityTypeListener implements EntityTypeListenerInterface {
       $storage->onEntityTypeCreate($entity_type);
     }
 
-    $this->eventDispatcher->dispatch(EntityTypeEvents::CREATE, new EntityTypeEvent($entity_type));
-
     $this->entityLastInstalledSchemaRepository->setLastInstalledDefinition($entity_type);
     if ($entity_type->entityClassImplements(FieldableEntityInterface::class)) {
       $this->entityLastInstalledSchemaRepository->setLastInstalledFieldStorageDefinitions($entity_type_id, $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id));
     }
+
+    $this->eventDispatcher->dispatch(EntityTypeEvents::CREATE, new EntityTypeEvent($entity_type));
   }
 
   /**
@@ -94,9 +94,9 @@ class EntityTypeListener implements EntityTypeListenerInterface {
       $storage->onEntityTypeUpdate($entity_type, $original);
     }
 
-    $this->eventDispatcher->dispatch(EntityTypeEvents::UPDATE, new EntityTypeEvent($entity_type, $original));
-
     $this->entityLastInstalledSchemaRepository->setLastInstalledDefinition($entity_type);
+
+    $this->eventDispatcher->dispatch(EntityTypeEvents::UPDATE, new EntityTypeEvent($entity_type, $original));
   }
 
   /**
@@ -116,9 +116,9 @@ class EntityTypeListener implements EntityTypeListenerInterface {
       $storage->onEntityTypeDelete($entity_type);
     }
 
-    $this->eventDispatcher->dispatch(EntityTypeEvents::DELETE, new EntityTypeEvent($entity_type));
-
     $this->entityLastInstalledSchemaRepository->deleteLastInstalledDefinition($entity_type_id);
+
+    $this->eventDispatcher->dispatch(EntityTypeEvents::DELETE, new EntityTypeEvent($entity_type));
   }
 
   /**
@@ -135,12 +135,12 @@ class EntityTypeListener implements EntityTypeListenerInterface {
     }
 
     if ($sandbox === NULL || (isset($sandbox['#finished']) && $sandbox['#finished'] == 1)) {
-      $this->eventDispatcher->dispatch(EntityTypeEvents::UPDATE, new EntityTypeEvent($entity_type, $original));
-
       $this->entityLastInstalledSchemaRepository->setLastInstalledDefinition($entity_type);
       if ($entity_type->entityClassImplements(FieldableEntityInterface::class)) {
         $this->entityLastInstalledSchemaRepository->setLastInstalledFieldStorageDefinitions($entity_type_id, $field_storage_definitions);
       }
+
+      $this->eventDispatcher->dispatch(EntityTypeEvents::UPDATE, new EntityTypeEvent($entity_type, $original));
     }
   }
 

core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php

@@ -85,10 +85,10 @@ class FieldStorageDefinitionListener implements FieldStorageDefinitionListenerIn
       $storage->onFieldStorageDefinitionCreate($storage_definition);
     }
 
-    $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));
   }
 
   /**
@@ -104,10 +104,10 @@ class FieldStorageDefinitionListener implements FieldStorageDefinitionListenerIn
       $storage->onFieldStorageDefinitionUpdate($storage_definition, $original);
     }
 
-    $this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::UPDATE, new FieldStorageDefinitionEvent($storage_definition, $original));
-
     $this->entityLastInstalledSchemaRepository->setLastInstalledFieldStorageDefinition($storage_definition);
     $this->entityFieldManager->clearCachedFieldDefinitions();
+
+    $this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::UPDATE, new FieldStorageDefinitionEvent($storage_definition, $original));
   }
 
   /**
@@ -133,10 +133,10 @@ class FieldStorageDefinitionListener implements FieldStorageDefinitionListenerIn
       $storage->onFieldStorageDefinitionDelete($storage_definition);
     }
 
-    $this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::DELETE, new FieldStorageDefinitionEvent($storage_definition));
-
     $this->entityLastInstalledSchemaRepository->deleteLastInstalledFieldStorageDefinition($storage_definition);
     $this->entityFieldManager->clearCachedFieldDefinitions();
+
+    $this->eventDispatcher->dispatch(FieldStorageDefinitionEvents::DELETE, new FieldStorageDefinitionEvent($storage_definition));
   }
 
 }

core/modules/system/tests/modules/entity_test/entity_test.services.yml

@@ -1,7 +1,7 @@
 services:
   entity_test.definition.subscriber:
     class: Drupal\entity_test\EntityTestDefinitionSubscriber
-    arguments: ['@state']
+    arguments: ['@state', '@entity.last_installed_schema.repository']
     tags:
       - { name: event_subscriber }
   cache_context.entity_test_view_grants:

core/modules/system/tests/modules/entity_test/src/EntityTestDefinitionSubscriber.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\entity_test;
 
+use Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface;
 use Drupal\Core\Entity\EntityTypeEvents;
 use Drupal\Core\Entity\EntityTypeEventSubscriberTrait;
 use Drupal\Core\Entity\EntityTypeInterface;
@@ -28,6 +29,13 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    */
   protected $state;
 
+  /**
+   * The last installed schema repository.
+   *
+   * @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface
+   */
+  protected $entityLastInstalledSchemaRepository;
+
   /**
    * Flag determining whether events should be tracked.
    *
@@ -38,8 +46,9 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
   /**
    * {@inheritdoc}
    */
-  public function __construct(StateInterface $state) {
+  public function __construct(StateInterface $state, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository) {
     $this->state = $state;
+    $this->entityLastInstalledSchemaRepository = $entity_last_installed_schema_repository;
   }
 
   /**
@@ -53,6 +62,9 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    * {@inheritdoc}
    */
   public function onEntityTypeCreate(EntityTypeInterface $entity_type) {
+    if ($this->entityLastInstalledSchemaRepository->getLastInstalledDefinition($entity_type->id())) {
+      $this->storeDefinitionUpdate(EntityTypeEvents::CREATE);
+    }
     $this->storeEvent(EntityTypeEvents::CREATE);
   }
 
@@ -60,6 +72,11 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    * {@inheritdoc}
    */
   public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
+    $last_installed_definition = $this->entityLastInstalledSchemaRepository->getLastInstalledDefinition($entity_type->id());
+    if ((string) $last_installed_definition->getLabel() === 'Updated entity test rev') {
+      $this->storeDefinitionUpdate(EntityTypeEvents::UPDATE);
+    }
+
     $this->storeEvent(EntityTypeEvents::UPDATE);
   }
 
@@ -74,6 +91,9 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    * {@inheritdoc}
    */
   public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
+    if (!$this->entityLastInstalledSchemaRepository->getLastInstalledDefinition($entity_type->id())) {
+      $this->storeDefinitionUpdate(EntityTypeEvents::DELETE);
+    }
     $this->storeEvent(EntityTypeEvents::DELETE);
   }
 
@@ -81,6 +101,9 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    * {@inheritdoc}
    */
   public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $storage_definition) {
+    if (isset($this->entityLastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions($storage_definition->getTargetEntityTypeId())[$storage_definition->getName()])) {
+      $this->storeDefinitionUpdate(FieldStorageDefinitionEvents::CREATE);
+    }
     $this->storeEvent(FieldStorageDefinitionEvents::CREATE);
   }
 
@@ -88,6 +111,10 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    * {@inheritdoc}
    */
   public function onFieldStorageDefinitionUpdate(FieldStorageDefinitionInterface $storage_definition, FieldStorageDefinitionInterface $original) {
+    $last_installed_definition = $this->entityLastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions($storage_definition->getTargetEntityTypeId())[$storage_definition->getName()];
+    if ((string) $last_installed_definition->getLabel() === 'Updated field storage test') {
+      $this->storeDefinitionUpdate(FieldStorageDefinitionEvents::UPDATE);
+    }
     $this->storeEvent(FieldStorageDefinitionEvents::UPDATE);
   }
 
@@ -95,6 +122,9 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
    * {@inheritdoc}
    */
   public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $storage_definition) {
+    if (!isset($this->entityLastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions($storage_definition->getTargetEntityTypeId())[$storage_definition->getName()])) {
+      $this->storeDefinitionUpdate(FieldStorageDefinitionEvents::DELETE);
+    }
     $this->storeEvent(FieldStorageDefinitionEvents::DELETE);
   }
 
@@ -130,4 +160,30 @@ class EntityTestDefinitionSubscriber implements EventSubscriberInterface, Entity
     }
   }
 
+  /**
+   * Checks whether the installed definitions were updated before the event.
+   *
+   * @param string $event_name
+   *   The event name.
+   *
+   * @return bool
+   *   TRUE if the last installed entity type of field storage definitions have
+   *   been updated before the was fired, FALSE otherwise.
+   */
+  public function hasDefinitionBeenUpdated($event_name) {
+    return (bool) $this->state->get($event_name . '_updated_definition');
+  }
+
+  /**
+   * Stores the installed definition state for the specified event.
+   *
+   * @param string $event_name
+   *   The event name.
+   */
+  protected function storeDefinitionUpdate($event_name) {
+    if ($this->trackEvents) {
+      $this->state->set($event_name . '_updated_definition', TRUE);
+    }
+  }
+
 }

core/modules/system/tests/modules/entity_test_update/entity_test_update.services.yml

@@ -0,0 +1,6 @@
+services:
+  entity_test_update.entity_schema_listener:
+    class: Drupal\entity_test_update\EventSubscriber\EntitySchemaSubscriber
+    arguments: ['@entity.definition_update_manager', '@state']
+    tags:
+      - { name: 'event_subscriber' }

core/modules/system/tests/modules/entity_test_update/src/EventSubscriber/EntitySchemaSubscriber.php

@@ -0,0 +1,74 @@
+<?php
+
+namespace Drupal\entity_test_update\EventSubscriber;
+
+use Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface;
+use Drupal\Core\Entity\EntityTypeEventSubscriberTrait;
+use Drupal\Core\Entity\EntityTypeInterface;
+use Drupal\Core\Entity\EntityTypeListenerInterface;
+use Drupal\Core\Field\BaseFieldDefinition;
+use Drupal\Core\State\StateInterface;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Defines a class for listening to entity schema changes.
+ */
+class EntitySchemaSubscriber implements EntityTypeListenerInterface, EventSubscriberInterface {
+
+  use EntityTypeEventSubscriberTrait;
+
+  /**
+   * The entity definition update manager.
+   *
+   * @var \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface
+   */
+  protected $entityDefinitionUpdateManager;
+
+  /**
+   * The state service.
+   *
+   * @var \Drupal\Core\State\StateInterface
+   */
+  protected $state;
+
+  /**
+   * Constructs a new EntitySchemaSubscriber.
+   *
+   * @param \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface $entityDefinitionUpdateManager
+   *   The entity definition update manager.
+   * @param \Drupal\Core\State\StateInterface $state
+   *   The state service.
+   */
+  public function __construct(EntityDefinitionUpdateManagerInterface $entityDefinitionUpdateManager, StateInterface $state) {
+    $this->entityDefinitionUpdateManager = $entityDefinitionUpdateManager;
+    $this->state = $state;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    return static::getEntityTypeEvents();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
+    // Only add the new base field when a test needs it.
+    if (!$this->state->get('entity_test_update.install_new_base_field_during_update', 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(new TranslatableMarkup('A new base field'));
+    $this->state->set('entity_test_update.additional_base_field_definitions', $definitions);
+
+    $this->entityDefinitionUpdateManager->installFieldStorageDefinition('new_base_field', 'entity_test_update', 'entity_test_update', $definitions['new_base_field']);
+  }
+
+}

core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php

@@ -14,6 +14,8 @@ use Drupal\Core\Field\BaseFieldDefinition;
 use Drupal\Core\Field\FieldException;
 use Drupal\Core\Field\FieldStorageDefinitionEvents;
 use Drupal\Core\Language\LanguageInterface;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
+use Drupal\entity_test\FieldStorageDefinition;
 use Drupal\entity_test_update\Entity\EntityTestUpdate;
 use Drupal\Tests\system\Functional\Entity\Traits\EntityDefinitionTestTrait;
 
@@ -835,28 +837,47 @@ class EntityDefinitionUpdateTest extends EntityKernelTestBase {
     $event_subscriber->enableEventTracking();
 
     // Test field storage definition events.
-    $storage_definition = current(\Drupal::service('entity_field.manager')->getFieldStorageDefinitions('entity_test_rev'));
-    $this->assertFalse($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::DELETE), 'Entity type delete was not dispatched yet.');
-    \Drupal::service('field_storage_definition.listener')->onFieldStorageDefinitionDelete($storage_definition);
-    $this->assertTrue($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::DELETE), 'Entity type delete event successfully dispatched.');
+    $storage_definition = FieldStorageDefinition::create('string')
+      ->setName('field_storage_test')
+      ->setLabel(new TranslatableMarkup('Field storage test'))
+      ->setTargetEntityTypeId('entity_test_rev');
+
     $this->assertFalse($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::CREATE), 'Entity type create was not dispatched yet.');
     \Drupal::service('field_storage_definition.listener')->onFieldStorageDefinitionCreate($storage_definition);
     $this->assertTrue($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::CREATE), 'Entity type create event successfully dispatched.');
+    $this->assertTrue($event_subscriber->hasDefinitionBeenUpdated(FieldStorageDefinitionEvents::CREATE), 'Last installed field storage definition was created before the event was fired.');
+
+    $updated_storage_definition = clone $storage_definition;
+    $updated_storage_definition->setLabel(new TranslatableMarkup('Updated field storage test'));
     $this->assertFalse($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::UPDATE), 'Entity type update was not dispatched yet.');
-    \Drupal::service('field_storage_definition.listener')->onFieldStorageDefinitionUpdate($storage_definition, $storage_definition);
+    \Drupal::service('field_storage_definition.listener')->onFieldStorageDefinitionUpdate($updated_storage_definition, $storage_definition);
     $this->assertTrue($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::UPDATE), 'Entity type update event successfully dispatched.');
+    $this->assertTrue($event_subscriber->hasDefinitionBeenUpdated(FieldStorageDefinitionEvents::UPDATE), 'Last installed field storage definition was updated before the event was fired.');
+
+    $this->assertFalse($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::DELETE), 'Entity type delete was not dispatched yet.');
+    \Drupal::service('field_storage_definition.listener')->onFieldStorageDefinitionDelete($storage_definition);
+    $this->assertTrue($event_subscriber->hasEventFired(FieldStorageDefinitionEvents::DELETE), 'Entity type delete event successfully dispatched.');
+    $this->assertTrue($event_subscriber->hasDefinitionBeenUpdated(FieldStorageDefinitionEvents::DELETE), 'Last installed field storage definition was deleted before the event was fired.');
 
     // Test entity type events.
     $entity_type = $this->entityTypeManager->getDefinition('entity_test_rev');
+
     $this->assertFalse($event_subscriber->hasEventFired(EntityTypeEvents::CREATE), 'Entity type create was not dispatched yet.');
     \Drupal::service('entity_type.listener')->onEntityTypeCreate($entity_type);
     $this->assertTrue($event_subscriber->hasEventFired(EntityTypeEvents::CREATE), 'Entity type create event successfully dispatched.');
+    $this->assertTrue($event_subscriber->hasDefinitionBeenUpdated(EntityTypeEvents::CREATE), 'Last installed entity type definition was created before the event was fired.');
+
+    $updated_entity_type = clone $entity_type;
+    $updated_entity_type->set('label', new TranslatableMarkup('Updated entity test rev'));
     $this->assertFalse($event_subscriber->hasEventFired(EntityTypeEvents::UPDATE), 'Entity type update was not dispatched yet.');
-    \Drupal::service('entity_type.listener')->onEntityTypeUpdate($entity_type, $entity_type);
+    \Drupal::service('entity_type.listener')->onEntityTypeUpdate($updated_entity_type, $entity_type);
     $this->assertTrue($event_subscriber->hasEventFired(EntityTypeEvents::UPDATE), 'Entity type update event successfully dispatched.');
+    $this->assertTrue($event_subscriber->hasDefinitionBeenUpdated(EntityTypeEvents::UPDATE), 'Last installed entity type definition was updated before the event was fired.');
+
     $this->assertFalse($event_subscriber->hasEventFired(EntityTypeEvents::DELETE), 'Entity type delete was not dispatched yet.');
     \Drupal::service('entity_type.listener')->onEntityTypeDelete($entity_type);
     $this->assertTrue($event_subscriber->hasEventFired(EntityTypeEvents::DELETE), 'Entity type delete event successfully dispatched.');
+    $this->assertTrue($event_subscriber->hasDefinitionBeenUpdated(EntityTypeEvents::DELETE), 'Last installed entity type definition was deleted before the event was fired.');
   }
 
   /**

core/tests/Drupal/KernelTests/Core/Entity/FieldableEntityDefinitionUpdateTest.php

@@ -151,13 +151,17 @@ class FieldableEntityDefinitionUpdateTest extends EntityKernelTestBase {
       $this->assertEntityData($initial_rev, $initial_mul);
     }
 
+    // Enable the creation of a new base field during a fieldable entity type
+    // update.
+    $this->state->set('entity_test_update.install_new_base_field_during_update', TRUE);
+
     // Simulate a batch run since we are converting the entities one by one.
     $sandbox = [];
     do {
       $this->entityDefinitionUpdateManager->updateFieldableEntityType($updated_entity_type, $updated_field_storage_definitions, $sandbox);
     } while ($sandbox['#finished'] != 1);
 
-    $this->assertEntityTypeSchema($new_rev, $new_mul);
+    $this->assertEntityTypeSchema($new_rev, $new_mul, TRUE);
     $this->assertEntityData($initial_rev, $initial_mul);
 
     $change_list = $this->entityDefinitionUpdateManager->getChangeList();
@@ -427,8 +431,20 @@ class FieldableEntityDefinitionUpdateTest extends EntityKernelTestBase {
    *   Whether the entity type is revisionable or not.
    * @param bool $translatable
    *   Whether the entity type is translatable or not.
+   * @param bool $new_base_field
+   *   (optional) Whether a new base field was added as part of the update.
+   *   Defaults to FALSE.
    */
-  protected function assertEntityTypeSchema($revisionable, $translatable) {
+  protected function assertEntityTypeSchema($revisionable, $translatable, $new_base_field = FALSE) {
+    // Check whether the 'new_base_field' field has been installed correctly.
+    $field_storage_definition = $this->entityDefinitionUpdateManager->getFieldStorageDefinition('new_base_field', $this->entityTypeId);
+    if ($new_base_field) {
+      $this->assertNotNull($field_storage_definition);
+    }
+    else {
+      $this->assertNull($field_storage_definition);
+    }
+
     if ($revisionable && $translatable) {
       $this->assertRevisionableAndTranslatable();
     }