Add events for matching entity hooks

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.

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.

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?

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.

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.

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.

Added #2553169: Dispatch bundle CRUD events as related.

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.

Here are tests written against a potential implementation.

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?

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

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.

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

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

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?

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.

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 :(

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.

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.

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

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.

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?

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

+++ 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

@phenaproxima
Great idea!

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!

+++ 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;
}

?

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?

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"?

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.

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

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.

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.

Btw Here's related issue about event classes #2825358: Event class constants for event names can cause fatal errors when a module is installed

Re-roll.

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;

+1 for #52

please please please can we get rid of enity hooks

reroll of #52

1. +++ b/core/modules/system/tests/modules/entity_test_event/src/EventSubscriber/TestEventSubscriber.php
   @@ -0,0 +1,44 @@
   +class TestEventSubscriber implements EventSubscriberInterface {
   ...
   +  public function onInsert(EntityEvent $event) {
   ...
   +  public function onUpdate(EntityEvent $event) {
   ...
   +  public function onDelete(EntityEvent $event) {
   

   Doc comments missing

2. +++ b/core/modules/system/tests/modules/entity_test_event/src/EventSubscriber/TestEventSubscriber.php
   @@ -0,0 +1,44 @@
   +  public static function getSubscribedEvents() {
   ...
   +  }
   +
   +
   +}
   

   2 lines after the method. should be 1

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityEventsTest.php
   @@ -0,0 +1,64 @@
   +class EntityEventsTest extends KernelTestBase {
   ...
   +  public function testEventsInsert() {
   ...
   +  public function testEventsUpdate() {
   ...
   +  public function testEventsDelete() {
   

   Doc comments missing

Added comments/docblocks and addressed linter issues.

Looks good. +1 for RTBC

It still needs profiling & consensus on event names

An attempt to really use new API, still not clear how it should subscribe to entity-type-specific events
Probably event should have helper to extract hook name for BC

#64 for 8.6.x

Testbot

Just fixed coding standards for #64.

Just fixed coding standards for #66.

#70 for 8.7.x without changes in SqlContentEntityStorageTest.php

Interesting approach from sf-land https://romaricdrigon.github.io/2019/08/09/domain-events

Just to be referenced here, "there is a contrib for that": https://www.drupal.org/project/entity_events

Ofc, it would be better if this would be implemented/supported in core soon.

I wasn't aware of that project. What about a merge between the two projects?

Rerolled from #71

Trying to re-roll up to 9.1

Rerolled

Signature of EventDispatcherinterface::dispatch() has changed

Ignore

The 'core_version_requirement' constraint (9.1.0-dev) requires the 'core' key not be set in core/modules/system/tests/modules/entity_test_event/entity_test_event.info.yml 

The naming conventions adopted here probably need to allow for the fact that there is serious discussion about adding additional pre/post hooks that are OUTSIDE of the database transaction: #2992426: Add entity methods and hooks for executing pre/post code outside of the save transaction

We have been usting the patch in #85 for quite some time now with 0 problems.
Entities we have tested the patch on are core entities: Users, Comments, Taxonomy Terms | and commerce entities: Product, Variation, Attributes.

I will be back on this when/if we use it with more entities.

For now +1 for rtbc on this :P.

Not really sure what is happening with the PR and stuff, I have been using #85 for a long time and had no problems.

So for people like me, here is a reroll of #85 for 9.5.x.

#85 does not work for me when applied for 9.5.x. Does anyone has a patch that does? Thank you

#97 patch that was created for 10.x, worked for me for 9.5.x

Maybe helpful for someone to know

Fixed CCF for #97.
Please review.

Try To Fix The Failed CMF #97 Patch . And Add Interdiff between the #97-101.

Try To Fix The Failed CMF #101 Patch . And Add Interdiff between the #101-102

+++ b/core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
@@ -632,7 +632,9 @@
+/**
+ *
+ */

This could actually use some content. An empty PHPDoc doesn't add much value.

Legitimately curious, was #102 just an automated update without reviewing it? The empty comment is a code smell for a rote contribution.

That is quite possible indeed. I've seen a lot of re-rolls recently which were not good, omitting new files and things like adding comments that don't add any value.

Tried to fix CCF for #102.

Instead of removing the PHPDoc you should have added some content/text in it.

addressed the #107 Comment. upload The patch And Interdiff between.

Fix the Failed CMD Patch of Mine.Remove Failed Patch.

I have a project that uses a significant number of these hooks and it would be much nicer if they could be events instead.

What would also be good is if we could have a base EntityUpdateEvent (for all entity types), and extend that to NodeUpdateEvent (for nodes only), and subscribers on EntityUpdateEvent would also receive NodeUpdateEvent. Unfortunately, Symfony does not support this pattern as far as I can tell: https://github.com/symfony/symfony/pull/32079

I find it more challenging https://git.drupalcode.org/project/hux/-/blob/1.x/src/HuxReplacementHook...

@longwave please elaborate "these hooks" dowsides

@andypost all my code is written as services and so the hooks end up being

/**
 * Implements hook_node_update().
 */
function MODULE_node_update(NodeInterface $node) {
  if (in_array($node->bundle(), ['x', 'y'])) {
    \Drupal::service('MODULE.SERVICE')->method($node);
  }
}

It would be cleaner if I could just inject this service into an event listener.

There is a lot of activity around hooks via PHP attributes issue (#3366083: [META] Hooks via attributes on service methods (hux style)). I think it's time to make a final decision about which way we are going to replace the current hooks system.
So far we've got the following options.

1. Events via Symfony Event Dispatcher
2. Hooks via PHP attributes
3. Nothing (keep the current hook implementation)

This is required to allow starting work on concrete implementation of the new event/hook system. I am not sure on what is the best way to get the resolution. Guess it needs to be done by framework managers.

Forget !4340...

I moved the changes of !210 to 11.x in !4423
I will start working on the review of @claudiucristea, unless someone beats me to it, or the issue ends up getting dropped.
Having said that.. has there been any discussion on the matter?

Attaching a patch for 10.2.2 (Based on !4423) (Probably applies to older versions as well, that #97 does not cover)

This needs to be an MR against 11.x at this point.