Allow code to respond to entity/field schema changes

Definitely +1 on this: at a first glance it looks like the right thing to do IMO.

One doubt I had was whether using hooks or events, since we do not have a clear pattern yet AFAIK. However since all the involved code is OO, I guess events would be a better fit.

Another one was about registration: handlers are not instantiated by the DIC, are there other convenient registration approaches we could leverage?

Bump up, given that this blocks #2341323: Adapt the references field / table names in views, when corresponding entity schema changes if I understand it right.

It definitely blocks itself. Edit, and yeah likely the views issue as well, was thinking similar.

Also changing the existing methods to events would have to be beta deadline if at all.

Adding an event on top of the on* methods would probably work, so this narrowly avoids being a beta blocker.

Also re-titling, we might use an event here but that's not the reason this issue is critical, just implementation detail.

So just to clarify #4, the best solution to this issue is beta deadline, and it's not a beta blocker because we could... keep a redundant API to keep BC?

Tagging as a priority for a pre-AMS beta sprint, for sure. Should we actually postpone #2341323: Adapt the references field / table names in views, when corresponding entity schema changes on this or is there another way to solve that?

This is just a rough start for the field storage definitions, to play around with ideas.

@xjm yes...

Spoke with Daniel about this: we agreed to proceed with events and make the EM proxy the call to handlers implementing EntityTypeListenerInterface / FieldStorageDefinitionListenerInterface.

First draft. This is critical so not beta deadline, I guess.

This should be more or less complete: it provides two traits that allow EntityTypeListenerInterface and FieldStorageDefinitionListenerInterface methods to keep their signature, while being used as listener methods. This means that entity handlers do not need to depend on event objects. This is true also for modules needing to notify the entity manager manually: they can keep calling notification methods the usual way, without needing to know anything about Symfony's event system.

Traits allow this code to be re-used by other subsystems, like for instance Views, that just need to implement the entity/field listener interfaces.

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeEvent.php
   @@ -0,0 +1,83 @@
   +class EntityTypeEvent extends GenericEvent {
   

   It should be final.

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
   @@ -0,0 +1,83 @@
   +class FieldStorageDefinitionEvent extends GenericEvent {
   

   Same here.

@jibran:

AFAIK final use is discouraged: is that a pattern I am not aware of?

Cool work!

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -825,10 +826,11 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        $event_dispatcher = \Drupal::service('event_dispatcher');
   

   it would be great to inject it, that is not so much additional work, if you ask me. It just makes it clear how "bad" the situation is. (same with entity manager maybe, but yeah this is already a mess.

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * Event name for field storage definition creation.
   +   *
   +   * @var string
   +   */
   +  const CREATE = 'field_storage.definition.create';
   +
   +  /**
   +   * Event name for field storage definition update.
   +   *
   +   * @var string
   +   */
   +  const UPDATE = 'field_storage.definition.update';
   +
   +  /**
   

   Some of the other patterns we use is to have a FooEvents class which has the constants and nothing else, feel free to change it, if you want.

3. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * The original field storage definition.
   +   *
   

   What about adding a small comment about that it is just available in case you update one.

4. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEventSubscriberTrait.php
   @@ -0,0 +1,76 @@
   + * Helper method for FieldStorageDefinitionListenerInterface.
   

   it is not really a method.

5. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEventSubscriberTrait.php
   @@ -0,0 +1,76 @@
   +  public static function getFieldStorageDefinitionEvents() {
   +    $event = array('onFieldStorageDefinitionEvent', 100);
   +    $events[FieldStorageDefinitionEvent::CREATE][] = $event;
   +    $events[FieldStorageDefinitionEvent::UPDATE][] = $event;
   +    $events[FieldStorageDefinitionEvent::DELETE][] = $event;
   +    return $events;
   +  }
   +
   +  /**
   +   * Listener method for any field storage definition event.
   +   *
   +   * @param \Drupal\Core\Field\FieldStorageDefinitionEvent $event
   +   *   The field storage definition event object.
   +   * @param string $event_name
   +   *   The event name.
   +   */
   +  public function onFieldStorageDefinitionEvent(FieldStorageDefinitionEvent $event, $event_name) {
   +    switch ($event_name) {
   +      case FieldStorageDefinitionEvent::CREATE:
   +        $this->onFieldStorageDefinitionCreate($event->getFieldStorageDefinition());
   +        break;
   +
   +      case FieldStorageDefinitionEvent::UPDATE:
   +        $this->onFieldStorageDefinitionUpdate($event->getFieldStorageDefinition(), $event->getOriginal());
   +        break;
   +
   +      case FieldStorageDefinitionEvent::DELETE:
   +        $this->onFieldStorageDefinitionDelete($event->getFieldStorageDefinition());
   +        break;
   +    }
   +  }
   +
   +  /**
   

   I really like this pattern.

Cool work!

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -825,10 +826,11 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        $event_dispatcher = \Drupal::service('event_dispatcher');
   

   it would be great to inject it, that is not so much additional work, if you ask me. It just makes it clear how "bad" the situation is. (same with entity manager maybe, but yeah this is already a mess.

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * Event name for field storage definition creation.
   +   *
   +   * @var string
   +   */
   +  const CREATE = 'field_storage.definition.create';
   +
   +  /**
   +   * Event name for field storage definition update.
   +   *
   +   * @var string
   +   */
   +  const UPDATE = 'field_storage.definition.update';
   +
   +  /**
   

   Some of the other patterns we use is to have a FooEvents class which has the constants and nothing else, feel free to change it, if you want.

3. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * The original field storage definition.
   +   *
   

   What about adding a small comment about that it is just available in case you update one.

4. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEventSubscriberTrait.php
   @@ -0,0 +1,76 @@
   + * Helper method for FieldStorageDefinitionListenerInterface.
   

   it is not really a method.

5. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEventSubscriberTrait.php
   @@ -0,0 +1,76 @@
   +  public static function getFieldStorageDefinitionEvents() {
   +    $event = array('onFieldStorageDefinitionEvent', 100);
   +    $events[FieldStorageDefinitionEvent::CREATE][] = $event;
   +    $events[FieldStorageDefinitionEvent::UPDATE][] = $event;
   +    $events[FieldStorageDefinitionEvent::DELETE][] = $event;
   +    return $events;
   +  }
   +
   +  /**
   +   * Listener method for any field storage definition event.
   +   *
   +   * @param \Drupal\Core\Field\FieldStorageDefinitionEvent $event
   +   *   The field storage definition event object.
   +   * @param string $event_name
   +   *   The event name.
   +   */
   +  public function onFieldStorageDefinitionEvent(FieldStorageDefinitionEvent $event, $event_name) {
   +    switch ($event_name) {
   +      case FieldStorageDefinitionEvent::CREATE:
   +        $this->onFieldStorageDefinitionCreate($event->getFieldStorageDefinition());
   +        break;
   +
   +      case FieldStorageDefinitionEvent::UPDATE:
   +        $this->onFieldStorageDefinitionUpdate($event->getFieldStorageDefinition(), $event->getOriginal());
   +        break;
   +
   +      case FieldStorageDefinitionEvent::DELETE:
   +        $this->onFieldStorageDefinitionDelete($event->getFieldStorageDefinition());
   +        break;
   +    }
   +  }
   +
   +  /**
   

   I really like this pattern.

This adds explicit test coverage and should address #16. I did not inject the entity manager as that dependency is going to be removed when we refactor that code to dispatch a post schema event to which the entity manager itself can respond.

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionEvent.php
@@ -55,7 +38,8 @@ class FieldStorageDefinitionEvent extends GenericEvent {
+   *   (optional) The original field storage definition. This should be passed
+   *   only when updating

Unfinished PHP doc.

Reverted event dispatcher injection as it causes troubles during installation. Agreed with Daniel we need a dedicated issue to do that: #2350111: Introduce an event to decouple the module installer from the entity manager.

This should "fix" event dispatcher injection and address #16.2.
Hopefully this should be ready to go now.

The interdiff is against #18.

Fixed last failures

Awesome! Let's write an IS and a change record (not sure about the second bit).

Updated issue summary. Not sure about the CR, but I guess now that beta is out it would make sense, working on it.

CR draft at https://www.drupal.org/node/2350653

1. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -52,7 +52,8 @@ public function getServiceProviders($origin);
   +   *   A ContainerInterface instance
   

   Misses trailing point.

2. +++ b/core/lib/Drupal/Core/Entity/EntityTypeEvent.php
   @@ -0,0 +1,63 @@
   + * Defines a base class for all field storage definition events.
   ...
   +class EntityTypeEvent extends GenericEvent {
   

   Docs do not match class.

3. +++ b/core/lib/Drupal/Core/Entity/EntityTypeEvent.php
   @@ -0,0 +1,63 @@
   +   * The field storage definition.
   ...
   +  public function getEntityType() {
   

   wrong docs.

4. +++ b/core/lib/Drupal/Core/Entity/EntityTypeEvent.php
   @@ -0,0 +1,63 @@
   +   * The original field storage definition.
   ...
   +   * @return \Drupal\Core\Entity\EntityTypeInterface
   ...
   +  public function getOriginal() {
   

   wrong docs.

5. +++ b/core/lib/Drupal/Core/Entity/EntityTypeEvents.php
   @@ -0,0 +1,36 @@
   +  const CREATE = 'entity_type.definition.create';
   ...
   +  const UPDATE = 'entity_type.definition.update';
   ...
   +  const DELETE = 'entity_type.definition.delete';
   

   Do have event names to be prefixed by component/module or so?

6. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -86,24 +99,37 @@ class ModuleHandler implements ModuleHandlerInterface {
   +   *   The event dispatcher service.
   

   Should not say service - the fact that the dependency is is fulfilled via a service is irrelevant to the class :)

7. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -821,14 +840,13 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   -            $entity_manager->onEntityTypeCreate($entity_type);
   +            $this->eventDispatcher->dispatch(EntityTypeEvents::CREATE, new EntityTypeEvent($entity_type));
   

   The summary said, one can still call the EM to invoke the methods. But then the events are not dispatched. Should instead, the EM be in charge of dispatching the events maybe?

Good point about moving event dispatching into the entity manager, the attached patch does that. I removed event dispatcher injection from the module handler (sigh), but I kept the related code clean-up as it will make re-injecting the event dispatcher easier in #2350111: Introduce an event to decouple the module installer from the entity manager.

Let's see whether I broke something...

Edit: no idea about 5...

+++ b/core/core.services.yml
@@ -287,13 +287,13 @@ services:
+    arguments: ['%container.modules%', '@kernel', '@cache.bootstrap', '@event_dispatcher']

Forgot to revert this change.

Anything else needed before RTBC-ing this?

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -1084,4 +1091,23 @@ public function getName($module) {
+  /**
+   * Updates the kernel module list.
+   *
+   * @param string $module_filenames
+   *   The list of installed modules.
+   */
+  protected function updateKernel($module_filenames) {
+    // This reboots the kernel to register the module's bundle and its services
+    // in the service container. The $module_filenames argument is taken over as
+    // %container.modules% parameter, which is passed to a fresh ModuleHandler
+    // instance upon first retrieval.
+    $this->kernel->updateModules($module_filenames, $module_filenames);
+    // After rebuilding the container we need to update the injected
+    // dependencies.
+    $container = $this->kernel->getContainer();
+    $this->cacheBackend = $container->get('cache.bootstrap');
+  }

It is great to have this moved into a separate method, it just lets you think better about the problem.

Rerolled

still green.

I don't think we should be adding DependencySerializationTrait to things that are in the container. I've traced this back to the serialisation of the DisplayBag in a view. So I've added the trait to the DefaultPluginBag which means that the ViewsPluginManager is not serialised and so on...

Are there going to be followups to use EntityTypeEventSubscriberTrait and FieldStorageDefinitionEventSubscriberTrait in non test code in core?

Are there going to be followups to use EntityTypeEventSubscriberTrait and FieldStorageDefinitionEventSubscriberTrait in non test code in core?

Yep, Views will need those to react to table layout changes and adjust Views definitions accordingly.

Moreover those traits provide a BC layer, so I think it's useful to have them even if core is not using them yet, aside from tests.

Fixed the tests by using DependencySerializationTrait in the correct places.

Removing some unused uses - this patch looks good to me and I think my contributions minor enough that I can still rtbc.

Rerolled due to #2246647: Rename PluginBag to LazyPluginCollection

Committed/pushed to 8.0.x, thanks!

Forgot to remove a couple of @todos, sorry.

Committed 2681e4b and pushed to 8.0.x. Thanks!

Followup : #2411791: Provide empty methods rather than abstract methods in EntityTypeEventSubscriberTrait / FieldStorageDefinitionEventSubscriberTrait

For those of you who followed this issue - it introduced a bug which I have just reported in #3260520: GenericEvent is used improperly

Please review that bug report.