One of the most visible places where developers and site builders will still need to interact with hooks are the entity lifecycle hooks (presave/create/insert/update/predelete/delete).

Now that we have our own reasonably performant EventDispatcher, there is no reason not to fire an event every time the matching hook is fired.

Contrib can do this by overriding the invokeHook() method of the controller, but core should do this too as soon as possible.

Open question:
Use one generic event (EntityEvent with a getEntity() and getEntityTypeId() methods) or a class per entity type (NodeEvent with a getNode()), defined via the entity annotation. Or both? (use the generic event if the more specific one isn't declared).

The same choice will reflect on the event id as well.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

bojanz created an issue. See original summary.

Xano’s picture

Just a quick note: we shouldn't be adding this on the annotation. This is storage handler specific, which means that top-level annotation keys may become obsolete when the handler is changed. I know we've done similar things before, but it's confusing and bad practice.

I would probably argue for event-specific classes rather than entity type specific classes. I'd be happy to hear arguments for other cases, though.

bojanz’s picture

As I said on IRC, there's no point in event-specific classes when they all carry the same payload (the entity being acted upon).

dawehner’s picture

Well yeah I think for a start we should go with ContentEntityEvent | ConfigEntityEvent and then see whether there are actually usecases for more specific ones.
I doubt we can apply domain specific events given the way how entity API works and how people interact with that.

One question I would have, and I think we should think about is whether we should have a dedicated event dispatcher for those events. Its a total different domain compared to many other ones out there already.

Xano’s picture

As I said on IRC, there's no point in event-specific classes when they all carry the same payload (the entity being acted upon).

I must have missed that message. I'd argue the update event could technically be extended with the original entity as it was known to its storage before it was updated.

I do in all honesty miss the point of having different events for content and config entities. I assume you have use cases for this?

bojanz’s picture

I must have missed that message. I'd argue the update event could technically be extended with the original entity as it was known to its storage before it was updated.

Refactoring $entity->original is a big task and definitely out of scope for this issue.
There's an issue for it at #1480696: Move $entity->original to a separate hook argument.

I doubt we can apply domain specific events given the way how entity API works and how people interact with that.

One question I would have, and I think we should think about is whether we should have a dedicated event dispatcher for those events. Its a total different domain compared to many other ones out there already.

What do we gain by using a dedicated event dispatcher?

Using a generic event class means that all subscribers must be instantiated regardless of which entity type they are handling, which negates our previous attempts to optimize dispatching (by only instantiating what's needed).

EDIT: Okay, I am confusing things. The problem above is caused by having the same event ids. We can construct unique event ids ($provider.$entityTypeId.insert, etc) to get around this problem while still using a generic class, if we want to.

bojanz’s picture

Crell’s picture

There's no value-add to a new domain dispatcher unless they need to be namespaced separately. Symfony has one single domain dispatcher, plus one per-form in its form API. (Doctrine does its own thing, which is silly Doctrine stuff.) Let's keep it in the single dispatcher.

I'd say an event class per event makes the most sense, not per-entity type. Per-entity-type means you'd have to create a bunch of stub objects every time you define a new entity type, which seems like a waste. There's enough boilerplate to creating a new entity as is. We should have a separate class per event, though, so EntityPresaveEvent, EntityCreateEvent, EntityInsertEvent, etc. (Although the naming may need to be clearer than hooks; it's totally non-obvious which are pre-hooks and which are post-hooks in most cases. Let's name the events to be self-evident.)

For hooks, we have both entity-generic and type-specific hooks. Do we want to continue that for events? My knee-jerk response is yes for the reasons bojanz mentions in #6, but could be convinced otherwise.

Crell’s picture

Also, I don't see why this should vary by backend. Rather, these events should be fired consistently regardless of the storage engine. If I as a module developer can't rely on them being the same, then my use of a particular event locks me in to a given backend implementation implicitly. Let's not do that. If a storage driver doesn't implement events at the appropriate times, file a bug against the driver. No annotation needed.

bojanz’s picture

I'd say an event class per event makes the most sense, not per-entity type. Per-entity-type means you'd have to create a bunch of stub objects every time you define a new entity type, which seems like a waste. There's enough boilerplate to creating a new entity as is. We should have a separate class per event, though, so EntityPresaveEvent, EntityCreateEvent, EntityInsertEvent, etc.

Do we gain anything by having per-event classes if they all cary the same payload and have the same getters?
Core tends to share the event classes (ConfigCrudEvent, LocaleEvent) based on payload.
I went through the pain of creating an event class per entity type for Commerce because it slightly improves DX on the consumer side ($event->getOrder() is explicit and matches non-entity-lifecycle events I have). I'm not married to the idea, just want to make sure we document our reasoning here.

(Although the naming may need to be clearer than hooks; it's totally non-obvious which are pre-hooks and which are post-hooks in most cases. Let's name the events to be self-evident.)

As long as we have hooks, the event names should match. Let's go through that rename once we remove the hooks (D9 material).

Also, I don't see why this should vary by backend. Rather, these events should be fired consistently regardless of the storage engine. If I as a module developer can't rely on them being the same, then my use of a particular event locks me in to a given backend implementation implicitly. Let's not do that. If a storage driver doesn't implement events at the appropriate times, file a bug against the driver.

Agreed.

Crell’s picture

Re event per class: I think about it semantically. What event just happened? An event was just deleted. That's the event, so that's the event class name. They mean different things, even if they are by default extremely similar. Think of it like section/article/aside tags vs. all divs. :-) Also, they may not be identical. $event->getOriginalEntity() would only exist on the update events, for instance. Even if they're super similar, and maybe share a trait or two for reducing boilerplate, they're still semantically distinct.

Re naming: Ideally, we would build events that do NOT need to get renamed in D9. Build what we want it to be, make it as good as possible, and that makes the transition easier. Ideally for Drupal 9 those events... don't change at all. We just remove the deprecated hooks. (That assumes we don't switch to a CRAP-only model in D9, which IMO we should, but at least those events that we keep wouldn't change names.) Better and more self-evident naming would, I'd argue, encourage event usage over hook usage, which I would consider a good thing.

claudiu.cristea’s picture

fago’s picture

Also, they may not be identical. $event->getOriginalEntity() would only exist on the update events, for instance. Even if they're super similar, and maybe share a trait or two for reducing boilerplate, they're still semantically distinct.

Agreed, the events are not 100% the same so modelling them as separate classes fits better and will saves us from semantic troubles with small difference like getOriginalEntity. Thus +1 on separate event classes.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Active » Needs review
FileSize
4.09 KB

Here are tests written against a potential implementation.

Status: Needs review » Needs work

The last submitted patch, 16: 2551893-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
5.33 KB

The test module had to be moved, it wasn't being picked up.
Additionally, the entity has to be saved before it can be deleted.

I added constants to EntityEvents for the main hooks we care about.
However, any subclass of EntityStorageBase could call invokeHook for any arbitrary hook, and we won't have an event for it.
ContentEntityStorageBase does this, for hooks like hook_entity_field_values_init, hook_entity_translation_create, etc.

I added a hardcoded array, otherwise we could have done constant('EntityEvents::' . strtoupper($hook));

Any ideas there?

Status: Needs review » Needs work

The last submitted patch, 18: 2551893-entityevent-18.patch, failed testing.

The last submitted patch, 18: 2551893-entityevent-18.patch, failed testing.

Mixologic’s picture

CI error in this case is missing @group annotation in the test.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.89 KB
496 bytes

Aha! Did you figure that out from the test results somehow? Or just by looking at the patch. I couldn't see anything about @group in the console output.

Status: Needs review » Needs work

The last submitted patch, 22: 2551893-entityevent-22.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
    @@ -163,6 +163,18 @@ protected function setStaticCache(array $entities) {
    +      \Drupal::service('event_dispatcher')->dispatch($hook_to_event_map[$hook], new EntityEvent($entity));
    

    This \Drupal::service() call is causing a bunch of unit tests to fail.

  2. +++ b/core/modules/system/tests/modules/entity_test_event/entity_test_event.info.yml
    @@ -0,0 +1,3 @@
    +name: entity_test_event
    +type: module
    +core: 8.x
    

    Needs package: Testing

Mixologic’s picture

It's pretty much the goto reason for "DrupalCI is giving a failure, and I cant see anything in the logs to indicate why".

Right now, if the Error is immediately following the run-tests.sh --list, then it is almost certainly a missing @group tag:

00:00:26.880 php /var/www/html/core/scripts/run-tests.sh  --list --php /opt/phpenv/shims/php > /var/www/html/artifacts/testgroups.txt
00:00:26.881 Command created as exec id 7c7a86e9
00:00:27.516 HTTP/1.1 200 OK
00:00:27.517 Content-Type: application/vnd.docker.raw-stream
00:00:27.517 
00:00:27.517 
00:00:27.517 Error

We've got a plan to fix this so its not so obtuse: #2680713: Handle missing @groups

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
9.71 KB
dawehner’s picture

You rock tim, seriously!

  1. +++ b/core/lib/Drupal/Core/Entity/EntityEvents.php
    @@ -0,0 +1,64 @@
    +  /**
    +   * @todo.
    +   *
    +   * @Event
    +   *
    +   * @var string
    +   */
    +  const PRESAVE = 'entity.presave';
    

    What about postsave?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
    @@ -163,6 +163,18 @@ protected function setStaticCache(array $entities) {
    +    if (isset($hook_to_event_map[$hook])) {
    +      \Drupal::service('event_dispatcher')->dispatch($hook_to_event_map[$hook], new EntityEvent($entity));
    +    }
    

    Should we invoke them now after or before?

tim.plunkett’s picture

1) hook_entity_update and hook_entity_insert are the equivalent of hook_entity_postsave, see EntityStorageBase::doPostSave()
2) Aha yes, I think we discussed moving it after the hooks.

Moved the hacky $hook_to_event_map to be a constant on the EntityEvents class. Also documented all of them, and reordered to be in the order they are usually fired.

Status: Needs review » Needs work

The last submitted patch, 28: 2551893-entityevent-28.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.16 KB
1.72 KB

TIL that class constants can only contain arrays as of PHP 5.6.0, and since our minimum requirements are 5.5.9, can't use that approach :(

tim.plunkett’s picture

Or, here's this. Which could be even better since another storage class could cheat and add their own event name...
Interdiff is against #28.

claudiu.cristea’s picture

1) hook_entity_update and hook_entity_insert are the equivalent of hook_entity_postsave, see EntityStorageBase::doPostSave()

I like the idea of a postsave dispatcher. Too many times I faced the situation to implement both hook_entity_update() and hook_entity_insert() with the same code. In many cases you want the same reaction regardless if the entity is new or existing. IMO, the correct flow would be:

  1. new entity: presave > save > postsave > insert
  2. existing entity: presave > save > postsave

No update hook.

tim.plunkett’s picture

I opened #2798783: hook_entity_insert() and hook_entity_update() are confusing to discuss that, it's out of scope here IMO.

fago’s picture

I think we should design the events solid from scratch, such that they are easy to grapsh. I don't think having a a 1:1 map to hooks is required here, instead we can take the opportunity and make sure the align well. That said, I'd just go for:
pre/postCreate
pre/postSave
pre/postDelete

Related: Once this is done, add onChange() for reactions. -> #1729812: Separate storage operations from reactions

FYI: This is what doctrine does:
http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/referen...
They have persist for insert and update for update, but no common postSave.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityEvents.php
    @@ -0,0 +1,105 @@
    +   * This hook runs after a new entity object has just been instantiated.
    

    This is no hook any more that runs.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityEvents.php
    @@ -0,0 +1,105 @@
    +   * You can get the original entity object from $entity->original when it is an
    +   * update of the entity.
    

    Let's make sure the new API is solid from the start:
    $entity->original is no property of the entity. $original_entity is some temporary variable that is available during the operation. Imo, the event should provide that as second variable.

    ad #6:

    Refactoring $entity->original is a big task and definitely out of scope for this issue.
    There's an issue for it at #1480696: Move $entity->original to a separate hook argument.

    I'd agree that we should not re-factor it here, but we should define the API already such that we avoid unnecessary future changes. It's easy to add $entity->original to the event as another variable or as getter which just passes of to $entity->original for now. Then #1480696 can still do the re-factoring.

dawehner’s picture

I agree with fago. If we want to provide good quality in the future we should think about the right names. We add a new API so we should not start with something we want to deprecate in the future anyway.

@tim.plunkett
What are your thoughts about that?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aaronbauman’s picture

Seems like #2221347 (via #2798783) is in consensus that post-save is the right approach to replace hook_update and hook_insert. We just need to agree that the new Entity Event API is allowed to have non-parity with the old Entity Hook API.

But #1480696 is conflicted (and stalled 2+ years) about the right approach. If we want to keep this issue moving forward without postponing on 1480696, seems like Entity Event API needs to support ->original property.

also: bump

phenaproxima’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityEvents.php
@@ -0,0 +1,105 @@
+final class EntityEvents {
+
+  /**
+   * Maps entity hook names to event names.
+   *
+   * @var string[]
+   */
+  public static $hookToEventMap = [
+    'create' => self::CREATE,
+    'presave' => self::PRESAVE,
+    'insert' => self::INSERT,
+    'update' => self::UPDATE,
+    'predelete' => self::PREDELETE,
+    'delete' => self::DELETE,
+  ];

One thing I'd like to see is the ability to subscribe specifically to entity type-specific events. So there should be the ability to react to events like entity.create.node or entity.delete.user.

One possible approach is to add some static methods to this class which could generate the appropriate event names, so you could get the event name for deleting a user like this:

EntityEvents::delete('user')

The implementation could be as simple as:

return static::CREATE . '.' . $entity_type_id

dawehner’s picture

@phenaproxima
Great idea!

phenaproxima’s picture

Okay, here we go. This implements my idea from #38. No tests for that yet, though. And no interdiff, because #31 no longer applies to 8.4.x. Sorry!

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kim.pepper’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -167,6 +167,23 @@ protected function invokeHook($hook, EntityInterface $entity) {
+      $event_name = call_user_func([EntityEvents::class, $hook], $this->entityTypeId);

The EntityEvents class seems a bit over the top. Can we make this a bit simpler?

Maybe just

// e.g. EntityEventNameBuilder::buildEventName($operation, $entityType) ==> 'entity.create.node'

public static function buildEventName($operation, $entityType) {
  return 'event.' . $operation . '.' . $entityType;
}

?

phenaproxima’s picture

It is verbose, but consider how it will look when used:

public static function getSubscribedEvents() {
  return [
    EntityEvents::buildEventName('presave', 'view') => 'onViewPresave',
    EntityEvents::buildEventName('update', 'node') => 'onNodeUpdate',
  ];
}

vs.

public static function getSubscribedEvents() {
  return [
    EntityEvents::presave('view') => 'onViewPresave',
    EntityEvents::update('node') => 'onNodeUpdate',
  ];
}

IMHO the second way is a lot easier to read at a glance.

One possibility is to implement a magic __callStatic() method so that we can kind of have our cake and eat it too, but I don't know if that would be considered kosher. What do you think?

dawehner’s picture

One possibility is to implement a magic __callStatic() method so that we can kind of have our cake and eat it too, but I don't know if that would be considered kosher. What do you think?

Once you have __call, you end up having no autocomplete support and what not. Are we scared of the code "duplication"?

phenaproxima’s picture

Are we scared of the code "duplication"?

I am not, but I was trying to see if I could find an alternative that @kim.pepper might find more palatable.

kim.pepper’s picture

Ah, it's a minor nit pick. I thought having an event name builder at all was over the top.

dawehner’s picture

That's actually a good point. You have a string which will never change, why would need to have a method for that. There is not really much of an abstraction we have as a method.

phenaproxima’s picture

The string does change, because the event name for inserting a node is not the same as inserting a user. It's a thin layer of abstraction, for sure, but it's worth having in case we need to implement more complex event name-building logic at some point in the future.

Also, we should not expect developers to remember the format of an event name for a specific entity type. I can easily see people getting confused about whether it's supposed to be EntityEvents::INSERT . ':node' or EntityEvents::INSERT . '.node'. Wrapping these event names in simple methods removes the ambiguity.

Not to mention that, should we need to change the event name at any point in the future, having a method gives us the latitude to do that easily.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Issue tags: -Needs reroll
FileSize
13.77 KB

Re-roll.

olexyy.mails@gmail.com’s picture

Please look at:
https://www.drupal.org/sandbox/olexyymailsgmailcom/hook_manager

- it should be rather fast;
- it uses caching and static;
- priority is supported;
- we have progress in naming convention problems;
- we can inject services to classes that implement hooks;