Problem/Motivation
Module authors converting their procedural code to services would like to implement hooks in said services without procedural code.
Proposed resolution
One, the event bridge. In ModuleHandler:
public function invokeAll($hook, $args = array()) {
$event = new HookEvent("hook.$hook", $args)
\Drupal::service('event_dispatcher')->dispatch($event);
$return = $event->getResults();
// ... the rest of the function is unchanged.
}
this way people can write dependency injected services implementing a hook instead of procedural code. As we hope to attract new developers more accustomed to OOP, this is sort of important.
Alternatively we could put the hook definitions in a class like Drupal\System\HookDefinition\System
, one method per hook with the doxygen and with empty bodies. Classes would extend these definitions, register themselves into the service container with the tag hook
. On container compile, we could parse the classes of these services with existing Doctrine code to find the methods defined by them and store the hook => array-of-services list into the container and then invokeAll:
public function invokeAll($hook, $args = array()) {
$result = array();
$hook_method_name = Camelize($hook_name);
foreach (\Drupal::container()->getParameter("hook.$hook_name") as $service) {
// We could do something with the return value as well.
call_user_func_array(array(\Drupal::service($service), $hook_method_name), $args);
}
// ... the rest of the function is unchanged.
}
This would have the advantage of a) we could retire hooks even for D8 because we have a place where we can put the hook doxygen B) we never need to load all the hook definitions, unlike with the event bridge. It's a regression to load all that code, in D7 we added groups to hooks to avoid that. The main "against" here is that this not the familiar (?) event dispatcher but then again, the only difference here is that instead of tagging as 'event_subscriber' we need to tag with 'hook' and then subscription is automatic. Already the event_subscriber tag is a Drupalism, this isn't a big step ahead.
Remaining tasks
Update module handler as required
Create HookEvent
Convert remaining hook invocations that do not use ModuleHandler::invokeAll ie where module_implements is used instead. There are two kinds: one that wants to pass around references, these are easy to convert because invokeAll takes an array of arguments instead of a variable number of arguments and the array simply can contain the references. Those that want to record the module that returned certain information will need some consideration (permissions do this for uninstall). The problematic cases will likely be handled in a followup.
User interface changes
None
API changes
Module maintainers can implement hooks in their service classes.
ModuleHandler::moduleImplements likely becomes a protected method.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#51 | drupal_1972304_51.patch | 13 KB | Xano |
#51 | interdiff.txt | 1.42 KB | Xano |
#14 | with-eventdispatcher.d8.xhprof.txt | 826.5 KB | cweagans |
#14 | without-eventdispatcher.d8.xhprof.txt | 824.47 KB | cweagans |
Comments
Comment #0.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #0.1
larowlanUpdated issue summary.
Comment #0.2
larowlanUpdated issue summary.
Comment #1
tim.plunkettI'm unsure about "ModuleHandler::moduleImplements likely becomes a protected method" but otherwise, I think this is a very good idea, at least good enough to see some code!
Comment #2
chx CreditAttribution: chx commentedYou wont be able to call module_implements, it'll become an internal implementation detail. invokeAll (module_invoke_all) is the way to call every implementation of a hook.
Edit: this is the goal. Not necessarily in the first round.
Comment #2.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #3
Crell CreditAttribution: Crell commentedThis sounds quite promising to me, and I'd love to see code for it. +1
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedI would love to see that happening too.
What would work very nicely is:
So that you can implement a subscriber like:
From a consumer perspective, that's lovely.
Comment #5
chx CreditAttribution: chx commentedAlternative proposal: the event dispatcher model does not have a place to put documentation. So, let's change system.api.php into Drupal\System\HookDefinition\System.php or something like that and have classes extending those. The class would have methods with doxygen and empty function bodies. The implementation class would look like this:
So one implementation class per defining module -- views hook implementations in one class, taxonomy hook implementations in another etc. These classes would need to be registered into the container and tagged as hook.
So then we have a list of classes based on the tagged service definitions. We can use core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.php to find the methods therein and record an array ($hook_name => array($list_of_services)) parameter into the container much like the module_implements cache now.
This does not require including all the implementation classes in one go, ever which is also a distinct and important advantage over eventdispatcher.
Comment #6
Crell CreditAttribution: Crell commentedActually the convention in some parts of the Symfony world, which we've been sort of adopting, is to define a constant class that has the event names in it and documentation. Cf:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...
(Which is modeled on the KernelEvents class from HttpKernel.)
I think that works well enough on the documentation front.
"One class for all hooks" is a terrible hack of OO design, and not one I would support. I much prefer the original proposal. It's simple, flexible, and clean.
Comment #7
chx CreditAttribution: chx commentedIt's not one class for all hooks, it's one class per defining module. That's extensible and much smaller.
Where do you plan to put http://api.drupal.org/api/drupal/modules!system!system.api.php/function/... in the event subscriber model?
Comment #8
Crell CreditAttribution: Crell commentedThat's still less clean and less flexible than the original proposal. EventSubscribers let you put multiple listeners in the same object instance however you want, in whatever way makes sense for you.
And I think hook_page_build() is already slated for execution by SCOTCH... It would be replaced by a combination of a view listener and a wrapping _controller (like the kind we're wiring up with route enhancers now).
I still like #0/#4 better. (I'd prefix the hook name with hook. when turning it into an event to avoid namespace collisions, but that's a minor quibble.)
Comment #9
chx CreditAttribution: chx commentedThat's dodging the question, I could've picked any other hook. hook_help, hook_forms, hook_openid whatever. Where do you plan to put their doxygen?
Comment #10
chx CreditAttribution: chx commentedTo sum up: the class version allows for total removal of hooks, in D8 or D9 doesn't matter because it gives you a point to hold the doxygen. A move from .module to class is totally painless, it's just moving the function and changing the function name. It's much easier to implement a hook because your IDE will directly link you to the API documentation in the class parent. Finding hook implementations is the same as before -- you were searching for
^function.*_hook_name
or somesuch, now it's justfunction hookName
. It enforces modules firing hooks to document their hooks. It's all advantages. The disadvantages are minor: there's a little magic here in invoking these classes but it's truly minimal, if you get the EventDispatcher, you will understand how these are fired: if you tag a service 'event_subscriber' you need to manually register your events, if you tag something 'hook' the it registers the methods automaticallyThe event bridge has no point to put the doxygen on. It regresses from Drupal 7 that at one point you need to load all the hook implementations to find what implements what. In D7 we added grouping to avoid this. To find a hook implementation, you need to grep for the constant and hope it's only used in the getSubscribers method. A move from .module to .class requires the explicit registration of the methods. And the method names will never be consistent and your IDE won't help at all in what to name them or what arguments they get.
Comment #10.0
chx CreditAttribution: chx commentedupdated issue summary
Comment #10.1
chx CreditAttribution: chx commentedadded class method
Comment #11
xjmSome tags.
Comment #12
xjmWe discussed this issue yesterday with @chx, @Dries, @effulgentsia, @Moshe Weitzman, and myself. Here's Dries' summary of the approach to take, based on where we are currently at in the release cycle:
Comment #13
Crell CreditAttribution: Crell commentedStep 1: Yes.
Step 2: Needs benchmarks to know for sure. The overhead for a no-implementation event is, I think, fairly low, but it's there.
Comment #14
cweagansI just did some quick profiling, and IMO, we should fire an EventDispatcher event every time a hook is invoked, which basically results in a choice between our normal hook system and eventdispatcher.
xhprof files for before and after the patch are attached.
Summary: looks like it only adds 30k of memory usage and an extra ~350 function calls to call event dispatcher for invokeAll() call (and this is obviously a no-implementation event - just using the built-in GenericEvent for the sake of testing).
Based on that, can we move forward here?
Comment #15
cweagansAlso, those were run in a fresh install of a Drupal 8 site (installed with Standard) on the front page. No content, one user, etc.
Comment #16
chx CreditAttribution: chx commentedI think we could move forward here -- if many implementations turn out to be slow(er) than we will get #1972300: Write a more scalable dispatcher done.
Comment #17
Crell CreditAttribution: Crell commentedHere we go with take 1.
This patch adds a HookEvent, and fires it from invokeAll(). It also introduces a mechanism to handle returns from hook event listeners, so that we have parity with the current capabilities of hooks. These work in both the installer and normal runtime (ooo...). I then converted history_cron() to a listener. history_cron() is not exactly the best candidate for doing so, but it was dead-simple so allowed me to exercise the code a little. Other hooks could easily be converted to the same subscriber class, or history could get refactored to be a service that the events just talk to, or whatever. That's all out of scope for now. :-)
While I was at it, I fixed a bug in the parameter order for CachedModuleHandler's constructor, which was buggy. (Optional parameters must come at the end.)
Comment #18
Crell CreditAttribution: Crell commentedComment #19
chx CreditAttribution: chx commentedThat's a beginning, however, the big work here is getting rid of the module_implements loops which cweagans have voluntereed for (thanks!)
Also if you'd fire the event first you wouldn't need to merge it back.
Comment #20
cweagansPer IRC convo w/ chx and Crell, I'll be working on this for a few days (I'll have a solid block of hours to spend on it on Monday night).
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedThe approach in #17 fires the event after running all of the procedural implementations. #19 suggests to do it before. In either case though, if a developer chooses to use an event subscriber vs. a procedural implementation, they're not just making a stylistic choice, they're taking their implementation out of the expected per-module-weight order. I think that will create a lot of problems in the wild.
Related to this, why bridge in this direction? Wouldn't it make more sense to bridge the other way? Make callers of $moduleHandler->invokeAll() instead call $eventDispatcher->dispatch()? And create a Drupal-specific dispatcher that can invoke procedural implementations intermixed by priority/weight with OO subscribers?
Comment #23
Crell CreditAttribution: Crell commentedeffulgentsia: To answer the second question, I worry about the performance impact. invokeAll() is currently faster than dispatch(). We have ideas (see links above) for how to make dispatch faster, but since at least for now most modules will be using hooks, not hook events, that would be additional overhead. Also, changing every instance of $moduleHandler->invokeAll() to $dispatcher->dispatch() would be a lot of work when we're trying to minimize the amount of non-critical work we have to do.
As far as the order, I don't think it's as big a deal as you make it out to be. Most hook implementations are order-agnostic. When they're not, there's often a double-hook setup to use instead (hook_info / hook_info_alter, etc.). Leveraging module weights is rare; and in those cases that you care, events offer more fine-grained order control. The only place it would be an issue is "I have to run before module X's implementation", in which case you'd have to use a hook, no event. I think that's less common than "I have to run after module X's implementation", so it's better to make "before" the hook-only option rather than vice versa.
Comment #24
cweagansSo, some things came up and I'm definitely not going to have time to tackle this. If somebody wants to work through it, here's a todo list:
☐ core/modules/user/user.admin.inc: foreach (module_implements('permission') as $module) {
☐ core/modules/user/user.module: foreach (module_implements('permission') as $module) {
☐ core/modules/user/user.module: foreach (module_implements('permission') as $module) {
☐ core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.php: foreach (module_implements('permission') as $module) {
☐ core/modules/user/lib/Drupal/user/Plugin/views/filter/Permissions.php: foreach (module_implements('permission') as $module) {
☐ core/modules/user/lib/Drupal/user/Plugin/views/access/Permission.php: foreach (module_implements('permission') as $module) {
☐ core/modules/image/image.module: foreach (module_implements('image_effect_info') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeStorageController.php: foreach (module_implements('entity_load') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeStorageController.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeFormController.php: foreach (module_implements('node_validate') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeFormController.php: foreach (module_implements('node_submit') as $module) {
☐ core/modules/node/node.module: foreach (module_implements('node_info') as $module) {
☐ core/modules/search/search.module: foreach (module_implements('search_info') as $module) {
☐ core/modules/search/search.module: foreach (module_implements('search_preprocess') as $module) {
☐ core/modules/action/tests/action_loop_test/action_loop_test.module: foreach (module_implements('watchdog') as $module) {
☐ core/modules/rdf/rdf.install: $modules = module_implements('rdf_mapping');
☐ core/modules/rdf/rdf.module: foreach (module_implements('rdf_namespaces') as $module) {
☐ core/modules/rdf/rdf.module: $modules = module_implements('rdf_mapping');
☐ core/modules/file/file.module: foreach (module_implements('file_download_access') as $module) {
☐ core/modules/help/help.module: $modules = module_implements('help');
☐ core/modules/help/lib/Drupal/help/Tests/HelpTest.php: foreach (module_implements('help') as $module) {
☐ core/modules/field/field.crud.inc: foreach (module_implements('field_attach_purge') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_attach_form') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_storage_pre_load') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_attach_validate') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_attach_extract_form_values') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_storage_pre_insert') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_storage_pre_update') as $module) {
☐ core/modules/field/field.module: $modules = array_merge(module_implements('field_info'), module_implements('field_widget_info'));
☐ core/modules/field/field.module: foreach (module_implements('field_access') as $module) {
☐ core/modules/field/field.info.inc: foreach (module_implements('field_info') as $module) {
☐ core/modules/field/field.info.inc: foreach (module_implements('field_storage_info') as $module) {
☐ core/modules/filter/filter.module: foreach (module_implements('filter_info') as $module) {
☐ core/modules/system/system.module: foreach (module_implements($hook) as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.php: foreach (module_implements('theme') as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php: foreach (module_implements('theme') as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.php: foreach (module_implements('theme') as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php: foreach (module_implements('theme') as $module) {
☐ core/lib/Drupal/Core/Plugin/Discovery/InfoHookDecorator.php: foreach (module_implements($this->hook) as $module) {
☐ core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.php: foreach (module_implements($this->hook) as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageController.php: foreach (module_implements('entity_load') as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageController.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php: foreach (module_implements('entity_load') as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php: foreach (module_implements('entity_load') as $module) {
☐ core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/includes/common.inc: foreach (module_implements('cron') as $module) {
☐ core/includes/common.inc: foreach (module_implements('page_build') as $module) {
☐ core/includes/schema.inc: foreach (module_implements('schema') as $module) {
☐ core/includes/theme.inc: foreach (module_implements('theme') as $module) {
☐ core/includes/bootstrap.inc: foreach (module_implements('watchdog') as $module) {
☐ core/includes/menu.inc: foreach (module_implements('help') as $module) {
☐ core/includes/menu.inc: foreach (module_implements('menu') as $module) {
Some of those are going to be really tricky. It might be worth getting a few people together around a table at Drupalcon and just churn through the list.
Comment #25
Crell CreditAttribution: Crell commentedI'm unclear on how HookEvent as currently imagined would help with that situation... Can someone clarify?
Comment #26
catchYeah I also don't see the connection. Cleaning up hook invocations to use the ModuleHandler is a separate issue and something that should be done anyway.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedI worry about that too, but this issue suffers from that no matter what, since once we make invokeAll() call dispatch(), then it will suffer the performance of it. #14 indicates that it's not a problem for a home page with no content: I don't yet know how that will translate to other scenarios.
So are we saying we'll leave ModuleHandler::alter() not dispatching an event? Wouldn't the same motivations for using event subscribers instead of procedural code (DI, stylistic preference) apply to _alter() hooks too? And it is extremely common for modules to need to control order of _alter() hooks. The idea that as soon as ModuleX chooses to change their procedural _alter() implementation into an OOP one that ModuleY is then also required to do the same, just to be able to run later, is both unfair to ModuleY (hey, don't make me change my simple function into a class with a ton of boilerplate code) and to ModuleX (hey, I'd like to benefit from DI in this implementation, but wait, if I make the change, it might break all sites using this module in conjunction with any 1 of 10,000 modules that might have a procedural alter implementation that is currently running after mine).
Comment #28
cweagansFrom IRC:
chx, I wasn't sure how to respond to Crell here: http://drupal.org/node/1972304#comment-7409118 I figured that was something that was already hashed out somewhere as part of a longer-term goal and that we were just doing it in that issue. Is that something you can elaborate on?
http://drupal.org/node/1972304 => Add a HookEvent #1972304: Add a HookEvent => Drupal core, base system, normal, needs work, 27 comments, 19 IRC mentions
cweagans: if we put the hookevent in invokeall then we need all hooks call invokeall
cweagans: those that use module_implements are exempt currently
cweagans: that's not good
Oh! Right, I see
Okay. That was the missing part
Comment #29
Crell CreditAttribution: Crell commentedAh, I see. Although the reason those cases bypass invokeAll() is because they care about the module that is responding. I don't see how "just" using an event would help that situation. If anything it means we need to change the way the event works to track that, which is quite problematic since events are not 1:1 coupled to modules by design. (One module can have a dozen different listener methods in 3 different subscribers for the same event if it felt like it.)
Alex: I think I see what you're getting at; yes we should be adding events to alter(), too. (I thought it wasn't needed as that method would just wrap invokeAll(), but apparently I was wrong.) I don't know how you suggest to resolve the ordering question, though. Interweaving hooks and events together would be horribly complex since their ordering mechanisms are so vastly different. We've run into such before/after questions before (does form_FORM_ID_alter run before or after form_alter?), and the answer has always been "meh, pick one and deal with it." I see no way that we could do better than that here. Certainly I'd rather have that problem than not get HookEvent in at all because we couldn't figure that out.
IMO, event-second makes more sense (as I explained above) but if we end up with event-first, it's not the end of the world. *shrug*
Comment #30
chx CreditAttribution: chx commented> Although the reason those cases bypass invokeAll() is because they care about the module that is responding.
Sigh. I wish people would read the issue summary. There are *extremely* few that care about the module, the real reason those happen is because you can't do module_invoke_all('hookname', &$a) but you can do ->invokeAll('hookname', array(&$a)) because invokeAll happens to use an array and arrays are exempt from call-time-reference...
Those that do care about which module will need to be dealt with in a followup. I think that's user permissions anyways... and perhaps the theme registry.
Comment #31
catchI discussed this with chx a week or so ago and I'm still not sure about it. From performance, to dealing with dynamic hooks, to what extent it matters that people get a choice between events and hooks when building 8.x modules.
Overall the procedural code that concerns me in 8.x is the mountains of stuff still left in /core/includes, let alone all the procedural API functions left in modules.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedI agree with catch. Furthermore, there can be a D8 contrib module that swaps out the module_handler service to one that dispatches events, and it can then iterate on thorny questions like invocation order. The only thing that would sway me to think this is worth doing in D8 core is if we have real use cases within core where hook implementations are problematic (e.g., we need DI or something like that). Though, since we can have hook implementations invoke services, I'm not aware of where we have a DI need that can't be solved that way.
I also agree with #26 that we should clean up all hook invocations to actually use invokeAll() or alter(), rather than custom iterating function names in all the places identified in #24.
Comment #33
Crell CreditAttribution: Crell commentedThe big impetus for this issue is DX: Telling devs "sometimes you have to go all OO... other times you have to go PHP 4 procedural, have fun" is unkind. Bridging hooks to events lets people who want to just stay OO stay OO for a much bigger percentage of the time.
I'd love to see /includes turn into services; some of it has open issues to do that, much doesn't. I haven't had the bandwidth to dig into that rats nest yet except where it touches on other things I'm working on (eg, current_path(), etc.). That's separate from this issue, though.
Comment #34
catchIf you're writing a completely self-contained custom module then you'd be able to stick to OO. Well except for all the places which would still be procedural even with this change like preprocess.
If you're working on a complex site or patching contrib modules then you'll have to deal with "sometimes this happens in a hook, sometimes it happens in an event" for exactly the same thing, that doesn't feel like an improvement.
Comment #35
Xano#17: 1972304-hook-events.patch queued for re-testing.
Comment #37
XanoRe-roll.
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedit is either CoreSubscriber or CronSubscriber:P
Comment #40
XanoComment #42
andypostThis example is not good because we have no tests for the hook at all
Comment #42.0
andypostUpdated issue summary.
Comment #43
XanoRe-roll. I also chose to test the patch by adding a unit test for the event and extending the one for
ModuleHandler
instead of changing the History module.Next to that, I renamed
HookEvent::AddToReturn()
toHookEvent::setReturnValue()
andHookEvent::getReturn()
toHookEvent::getReturnValues()
to be more self-explanatory and consistent with other methods within Drupal.Comment #45
Xano43: drupal_1972304_43.patch queued for re-testing.
Comment #47
AaronBauman#43 doesn't apply cleanly
here's a re-roll to address changes in LocalTaskIntegrationTest.php and fuzz
Comment #49
Xano47: drupal_1972304_47.patch queued for re-testing.
Comment #51
XanoThe problem was an incorrect argument order for CachedModuleHandler. This caused a class instantiation error, to which Drupal responded by loading the maintenance theme, which in turn needed the module handler as well.
Comment #53
XanoThe exceptions are caused by #2170989: Migrate provides invalid arguments when invoking hooks.
I just realized that this patch will break
\Drupal\Core\Extension\ModuleHandlerInterface::implementsHook()
, as it allows modules to respond to hook invocations without having a procedural hook implementation. We may have to make\Drupal\Core\Extension\ModuleHandler::implementsHook()
check all event subscriber services and see what events they subscribe to.Comment #54
XanoAs described in the issue summary, this patch adds events to
invokeAll()
, and we can add them toalter()
too. However, there is no way to add them toinvoke()
without breaking BC.invoke()
expects a single return value, but allowing events there lets modules respond to the hook using a procedural implementation and an event listener, which means the return value potentially is two values instead.Then there is the question of
\Drupal\Core\Extension\ModuleHandlerInterface::getImplementations()
and\Drupal\Core\Extension\ModuleHandlerInterface::implementsHook()
, both of which associate procedural hook implementations with modules. Any code that relies on these may incorrectly assume that no module implements a particular hook, while all implementations are event listeners.The most straightforward solution is to get rid of any functionality that deals with module-specific hook implementations. These include:
\Drupal\Core\Extension\ModuleHandlerInterface::invoke()
\Drupal\Core\Extension\ModuleHandlerInterface::getImplementations()
\Drupal\Core\Extension\ModuleHandlerInterface::implementsHook()
\Drupal\Core\Extension\ModuleHandlerInterface::()
Any code that needs to implement a specific hook implementation either shouldn't use hooks at all (because the hook API is abused to support magic callbacks), or require return values to contain a
module
key that specifies the module the values belong to.This change would break backwards compatibility, but it does allow us to convert the thousands of lines of hook implementations we have in core, and more in contrib, to unit-testable OOP code.
My proposal is that we either do not introduce events at all, or we go all the way and make sure events can be fired for any hook implementation.
Comment #55
dawehnercweagens grepped for it before: https://drupal.org/comment/7408792#comment-7408792
In general we certainly have to evaluate the performance of the event system. Let's be honest. It is NO big deal to call from hooks to services.
Comment #56
Crell CreditAttribution: Crell commentedPerhaps we're going about this all wrong.
The long-term goal is to transition "event-ish" hooks away from hooks and toward the event system, and do so in a gradual way rather than a hard "Break all the things!" approach. Ideally, during at least some of the Drupal 8 cycle module developers could use either hooks or events to accomplish various tasks, and in Drupal 9 the events would keep working exactly as they already were while the hooks would go away. That would give people a LONG time to get around to converting their modules with both forward and backward compatibility.
That goal doesn't require the hook system or the event system to transparently "front" for the other. That's one way of doing it, but not the only.
Do we really *need* events to show up in implementsHook()? What do we actually get out of that? I think, as a practical matter, we probably don't need that.
Many modules now have a hook invocation, followed by a rules invocation. Two event systems that fire in parallel, but independently of each other. Rules implementations don't show up in module_implements(), and module implementations don't show up in a list of rules implementations. Why not just treat events the same way?
So take, eg, the entity event hooks. Just stick a call to an entity event next to them, design that the way we want it to be in both D8 and D9, and call it a day. Don't try to sledge-hammer port everything in one big BC-layer patch. Let them coexist peacefully but clearly communicate the longer term plan.
Or if we really want, we can centralize the event calls but accept that it's not going to be perfect parity... and that's OK. We don't care that an event listener doesn't look exactly like a hook implementation to the hook system. And that's OK.
Comment #57
XanoIMHO having events for some, but not all hook invocations is confusing and causes WTFs. It's not a disaster, but it will waste people's time because of the inconsistency and it will only be a partial solution anyway.
This is fine with me, but will not really aid in the smooth transition that this issue aims to accomplish: offer support for both hooks and events without anyone having to think about it.
If developers simply want dependency injection and the ability to unit test their code, @dawehner suggested they can create a service with 'hook implementations'. The real hook implementations will then be no more than one-line procedural wrappers for methods on services.
Comment #58
chx CreditAttribution: chx commentedSee the original post of #1331486: Move module_invoke_*() and friends to an Extensions class for the original vision on how these thing should look. The plan was to have invoke replace invokeAll and instead of getting a full list of implementation, just a isImplemented was to be exposed. If #1972300: Write a more scalable dispatcher happens then the isImplemented is very easy to do.
Comment #59
Crell CreditAttribution: Crell commentedXano: My point is that "without anyone having to think about it" ma be a pipe dream, given the oddities around implementation, ordering, discovery, etc. We may be better off in the long run not pretending that they're the same system, just supporting both in parallel for a time.
Comment #60
webchickWhenever I travel to an event and talk to a non-Drupal developer, everything about D8 makes sense to them except for hooks. I'd like to escalate the importance of this issue, or something like it, that allows us to introduce a transitional step that makes it easier for non-Drupal developers to approach our event system while not requiring re-writing all of core.
Comment #61
XanoI personally started accepting we won't be getting this in core, as we cannot reliably introduce this feature for all hooks. Introducing it partially will allow people to write event listeners (even though the events will not be properly classed) for some hooks, but it will cause developers having to look up which hook invocations support the event and which do not. This also limits us in refactoring the hook code, as the event integration does not depend on the hook itself, but on how it is invoked.
I do not mean to scare people away from this issue, but if there is indeed no way around these limitations, our time might be better spent introducing dedicated (classed) events for the most important hook invocations.
Comment #62
XanoAlso:
It is not our event system, but Symfony's, which uses specific classes for events. Introducing one single HookEvent class will technically work, but it will still require developers to read the hook documentation in order to figure out what the event does, as we cannot document that on the class. Apart from being a partial solution, this is also not fully in line with how Symfony does things.
Comment #63
larowlanComment #64
catchThis won't reduce that confusion though. It'll just mean that some code doing exactly the same thing lives in hook implementations, while some lives in events.
Comment #65
XanoAlso, if you just want dependency injection and testability, you can do something like http://drupalcode.org/project/payment.git/blob/refs/heads/8.x-2.x:/payme... and http://drupalcode.org/project/payment.git/blob/refs/heads/8.x-2.x:/payme....
Comment #66
chx CreditAttribution: chx commentedComment #67
benjy CreditAttribution: benjy commentedCouldn't this still go in for 8.1?
Comment #68
tim.plunkettWe can at least talk about it again then.
Comment #69
cweagansThis issue can't be completed in a reasonable way without solving #2616814: Delegate all hook invocations to ModuleHandler first.
Comment #70
benjy CreditAttribution: benjy at PreviousNext commentedI'm experimenting with this in contrib if anyone wants to give me any feedback: http://drupal.org/project/hooks
Comment #71
catchTagging for issue summary update. The patch/title here is adding an event, but the issue summary talks about tagged services which is what's being discussed over at #2237831: Allow module services to specify hooks.
Comment #76
larowlan8.1 is long out
Comment #78
olexyy.mails@gmail.com CreditAttribution: olexyy.mails@gmail.com commentedPlease 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;
Comment #79
colan#78: How does this differ from #70?
Comment #80
olexyy.mails@gmail.com CreditAttribution: olexyy.mails@gmail.com commentedTechnically differes from#70, this is not events but plugins. And yes, bonuses are the same plus performance.
Internally it works almost luke existing solution.
My implementation supports invoke all as well as alter.
This is oop hook implementation which should be faster than events.
In my module new hooks are defined and can be added as plugins, like blocks or entities.
The idea is rather simple:
Annotation to plugin contains list of canonical names for hooks with corresponding priority. (f.e. hook_theme => 0)
We need to implement this hook as method in camel case, so this is public function hookTheme();
This approach can be additional to main implementation, that makes system very suite and flexible.
Comment #82
No Sssweat CreditAttribution: No Sssweat commentedYou might find this contrib module handy, Hook Event Dispatcher.
Comment #83
colan#71: Should we mark this one as a duplicate of #2237831: Allow module services to specify hooks, or the other way around? I don't think we need both.
Comment #84
tim.plunkettI reopened this issue 4 years ago. I don't think I needed to, after all.
Events can be added when they are appropriate, and hooks are still relevant on their own.
Deprecating individual hooks and replacing them with events could also be done on a case-by-case basis.