Add events for matching entity hooks

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.

The last submitted patch, , failed testing. View results →

The last submitted patch, 110: 2551893-109.patch, failed testing. View results →

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 ( 🌱 [META] Hooks via attributes on service methods (hux style) Active ). 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?

Marios Anagnostopoulos → changed the visibility of the branch 2551893-add-entity-events to hidden.

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.

This is fixed in 📌 OOP hooks using event dispatcher Needs review :D

Moving to duplicate just so people don't think there was a commit here.

Is this really a duplicate? I don't think it is.

This issue is NOT about replacing hooks with events, it is about having events dispatched in addition to hooks so that an event-based programming model can be supported. Is that really being done in that other issue? There's no mention of that in the change record.

@tr the other issue is 266 comments long, it also has multiple related issues, did you read it/them before re-opening this one?

If you didn't read it, then why contradict the people who have read it and worked on this issue for the past decade without doing any research?

Yes I read it, I have been following it, and I don't see how it's a duplicate.

After reading through this issue and 📌 OOP hooks using event dispatcher Needs review , as suggested in a comment above 📌 OOP hooks using event dispatcher Needs review , it seems that there are two different approaches:

• Using attributes for hooks as is done in the hux module →
• Using events alongside hooks as is done in the Hook Event Dispatcher module →

These approaches do not seem to be mutually exclusive.

In 📌 OOP hooks using event dispatcher Needs review , core has chosen to adopt the hux module approach.

This issue is about event dispatching as is done in the Hook Event Dispatcher module, whereas 📌 OOP hooks using event dispatcher Needs review is about using PHP attributes to replace hooks (the change record → states In a later Drupal version (Drupal 12 at earliest) we expect procedural hooks no longer to be supported and then these services and these LegacyHook marked shims will need to be removed.)

So if this issue is closed, my question is: has a decision been made to NOT support emitting events in addition to hooks? I originally started following this issue because I use Hook Event Dispatcher → , which currently has over 16,000 other users. It would be nice to have a little more clarity regarding the "duplicate" status here.

So if this issue is closed, my question is: has a decision been made to NOT support emitting events in addition to hooks? I originally started following this issue because I use Hook Event Dispatcher, which currently has over 16,000 other users. It would be nice to have a little more clarity regarding the "duplicate" status here.

That's my question exactly, speaking from my perspective of the person who maintains Rules, which is 100% based on using Events.

With Rules, in D7, D8, D9, D10, D11.0, what we have to do is IMPLEMENT ENTITY CRUD HOOKS, then dispatch Events from within those hook implementations. Rules can of course integrate with ANY Event from ANY source, but Entity CRUD events are a main use-case.

Likewise, Hook Event Dispatcher and the Entity Events modules (and others) do something similar to Rules. With the difference that Rules doesn't just do Events, but also Conditions and Actions so you don't need to write your own Event Subscriber. But that's another topic ...

I was hoping this issue would result in a cleaner integration with core, where CORE would dispatch the Events. That would fill a very common use-case for workflows, and in my mind that is why this issue was opened. I can't speak for the original posters, but @bojanz has always been involved with Commerce and @fago of course it the author of Rules, so I'm guessing that their motivations in pursing this issue are similar to mine and @ptmkenny's. From the OP, "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."

BEFORE I re-opened this issue in #126 I read the ENTIRE body of 📌 OOP hooks using event dispatcher Needs review . But more importantly, I read the CODE to see EXACTLY what that issue was doing. Because Change Records are notoriously sparse in details.

NOW, however, I have in addition actually implemented Hooks using the new system - I have updated about 10 modules (including Rules) with about 50 hooks in total, with full testing, so I now have practical knowledge and experience in how to properly implement Hooks with the new system. And I can now say 100% that this issue IS NOT A DUPLICATE OF 📌 OOP hooks using event dispatcher Needs review . Specifically, I STILL have to implement the Entity CRUD hooks with the new system, and I STILL have to create and dispatch Events from these hooks. Basically nothing has changed from the perspective of a contributed module developer who wants to program with events.

And yes, I've tried to subscribe to the new, internal non-events named "drupal_hook.$hook", but that cannot be done unless you're implementing a Hook using the Hook attribute. Any other subscriber throws PHP errors.

So again, why the pushback and the hate from my observation that this issue should not be closed as a duplicate? It's NOT a duplicate. Perhaps there are other issues that duplicate this one, but 📌 OOP hooks using event dispatcher Needs review is not one of them.

This is not a judgement of the work done in 📌 OOP hooks using event dispatcher Needs review . If you like I will offer you proper obeisance for the hard work in that issue. But really, that's not the point here. Regardless of what was done there, that does not fix the issue HERE.

At the risk of being shunned, I'm going to re-open this issue. I would like to at least have the OPTION for core to dispatch Events - doing it in conjunction with Hooks is an obvious thing to do, especially now that Hooks are piggybacking off of the Symfony Event dispatcher. Dispatching events at the same time as invoking hooks is almost no overhead - Drupal is already determining whether there are listeners before calling the hooks, so all that needs to be done is to create Event objects (a few hundred bytes each) and dispatch these events to the listeners. Almost no overhead, yet it enables many use cases that have never been well-served by the core Drupal event model.

@catch, @nicxvan, Please explain why you think this IS a duplicate issue. It seems to me you made a mistake in closing it.

And, @catch, if you've even bothered to read this far:

If you didn't read it, then why contradict the people who have read it and worked on this issue for the past decade without doing any research?

You seem to think I'm disrespecting the people who worked on that issue. That's not the case, because I never said anything bad (or good) about that issue. I simply said it is NOT A DUPLICATE of what is proposed here. And frankly, YOU are disrespecting ME by totally ignoring my 17 year long contribution to Drupal and by making a snap judgement and accusation without reasonable consideration of and refutation of my argument that this is not a duplicate. At least do me the favor of addressing my concern directly rather than dismissing it outright with no explanation.

And BTW, it's "TR" not "tr". Don't reduce me to a machine name.

I think NW is more appropriate as we have a pull request (which needs a rebase) and some other tags which justify NW.

Also let's not get too emotional here. It's sometimes difficult to express sentiment in text. I think the real issue is that #124 was taken for granted without reading in detail (lot's of reading) both issues.

Let's continue in a positive way, as I also don't believe this is a duplicate, and I'm also using this approach on a few projects.

ptmkenny → changed the visibility of the branch 9.2.x to hidden.

ptmkenny → changed the visibility of the branch 2551893-add-entity-events-11x to hidden.

A decision was made to update the hook system to support methods on services, so that we don't have to convert everything to events. Drupal core will IMHO not add support for events and hooks for the same thing. Performance is one thing, but I think the bigger reason is cognitive load for developers, having to learn two different ways to do the same thing and two different ways to find and search hook implementations adds considerable complexity. And, there are fairly frequent use cases where some implementations need to be called in a certain order, supporting hooks and events would likely make that even harder if not impossible to mainain.

I understand that there are use cases that might be easier with an event subscriber, but I don't think it's the end of the world to add a few lines of glue code that passes the handful of fixed entity hooks through to Rules, a generic implementation might be possible with a decorated module handler service or so, but I didn't verify or think about whether or not that is useful.

Maybe won't fix is a better status, so lets go with that. Either way, the issue has now been closed by both a core maintainer and an entity system maintainer, so lets leave it at that.

PS: The drupal.org login process changes have lowercased everyones username, mine was also changed from Berdir to berdir. I am certain that using @tr wasn't meant to be disrespectful in any way, it's just your username now.

Re the username. There is now an KeyCloak feature request open for this: https://github.com/keycloak/keycloak/issues/32869

Re #134: Good! I usually use class resolver in hooks and that adds needless boilerplate. I think using services for hooks may also be annoying as some modules implement a lot of hooks so maybe we could just check if class exists in certain namespace, register and cache if it does and use the class resolver to instantiate? Do we already have an issue for that?

Just commenting here to maybe clear something up as well.

The OOP change makes all hooks event listeners, so if you need to you can use a service provider to tag services with the event listener tag for the drupal_hook.$event tag yourself, dynamically.

Event listeners are a little different than event subscribers, but you should be able to interact with specific hooks as needed.
https://symfony.com/doc/current/event_dispatcher.html

hey, how about hooks like field_values_init or translation_create. The second one is used by the path module here... and since this hook is not in the EntityEvents::$hookToEventMap array it's never being executed.

Because of that, I have a problem with saving URL aliases for translatable content (when I save a translation, its URL alias is not saved, it's empty after saving the translation, although it shows in the form).

I made a quick fix for that:

// Dispatch an event for the entity-level hook (i.e., hook_entity_$hook).
if (isset(EntityEvents::$hookToEventMap[$hook])) {
  $event_dispatcher->dispatch($event, EntityEvents::$hookToEventMap[$hook]);
} else {
  $this->moduleHandler()->invokeAll('entity_' . $hook, [$entity]);
}

... and it helped in my case, but I'm not sure if it's a correct way to go.

I'm using Drupal 10.3.9.