Delegate all hook invocations to ModuleHandler

I sort of feel like this is by design. Those places are generating those function calls because invoke doesn't work for them.

While there might be a couple of places where you could get rid of it, there are certainly also places where this is by design.
But having an effort to get rid of them is certainly not the worst idea.

If there are cases where invokeAll() doesn't work and a caller needs to do it themselves, that's a sure-fire sign that ModuleHandler needs a new method to handle that case, invokeAllWithFancyReturn() or somesuch. If modules need to hack around our API, it means the API is insufficient and we should improve it.

IMHO the way to move forward is to convert what is possible to be able to see what is left. Well, getImplementations() was part of the official API since ever.

getImplementations() gives you a list of modules that implement a hook. That does not mean that you are allowed to create a hook name out of this for yourself to call.

I think the main problem is that invoke() / invokeAll() does use call_user_func_array(), so won't work per reference.

However it is very possible to circumvent this generically:

https://github.com/LionsAd/service_container/blob/7.x-1.x/src/Legacy/Dru...

In theory all that is needed is the arguments to references thing.

Meanwhile I understand enough of PHP's internals to know that that is even safe to do - even still not officially documented.

In any case we need a replacement for invoke() that can change arguments - like alter() can do and still works for all kinds of callables, which is a challenge in itself - without using that trick.

Edit:

The megalith while ugly works, too - as for any $callable, you can just do:

$callable($arg_0, $arg_1);

etc.

https://github.com/LionsAd/service_container/blob/7.x-1.x/src/Legacy/Dru...

as also used here for speed reasons.

I could image that we change invoke / invokeAll to allow the first 3 arguments to be references - same as with _alter().

It is too late to change the core usage / hooks due to BC.

Or we need to introduce invokeMutableAll() or such ...

But probably e.g. hook_page_attachments() should have just returned the attachments instead of adding them to a variable that is changed - which is a non-use. Too bad I did not catch that during the introduction of that hook.

$ grep -A3 getImplementations -r core/ | egrep -v 'Tests|ModuleHandler'

gives a pretty good overview of what different usages we have.

So we could have getCallables($hook), which returns $callables, which would probably solve all use-cases easily.

As most could would not change at all, but just remove one line and change getImplementations to getCallables() and $module to $function.

#329012: Remove call_user_func_array() from ModuleHandler::invoke() + invokeAll() has a lot of different patches trying to do similar things with ModuleHandler::invokeAll() as was done with ModuleHandler::alter() - to make the call stack simpler, allow by-ref etc. etc. - there might be a couple of other issues for this too.

I think if we can solve the core use cases with one or two additional methods, it would be ideal if those methods also handled the actual calling of the functions too - makes it easier when debugging if all hook invocations come directly from the module handler.

getCallables() seems like a very flexible way to solve the immediate problem, just not sure if we want to encourage the self-calling of hook implementations at all - makes that a bit more official.

There is however also functions like rdf that make this tricky:

core/modules/rdf/rdf.module: foreach (\Drupal::moduleHandler()->getImplementations('rdf_namespaces') as $module) {
core/modules/rdf/rdf.module- $function = $module . '_rdf_namespaces';
core/modules/rdf/rdf.module- foreach($function() as $prefix => $namespace) {
core/modules/rdf/rdf.module- if (array_key_exists($prefix, $namespaces) && $namespace !== $namespaces[$prefix]) {

For those the new invokeAll thingy would need to take a callback to do something like that, but getCallables() would still work - tricky.

Thinking about this again some more the RDF use-case could also be satisfied with a normal invokeAll - but that uses array_merge_recursive, when it returns an array, so it would need to switch to objects instead as array_merge_recursive does not work in a standard map/reduce pattern.

So another idea I had was to use modern PHP 5.5 iterators or generators.

$results = $moduleHandler->getIterator('rdf_namespaces');

foreach ($results as $result) {
  // do something - $result is computed on the fly via the callback
}

But that does not feel easier and still would not allow alters.

So the problem with using invoke and allowing altering is that:

With the proposal in the hooks issue, services could have hooks, but that means that - even while a service belongs to a module or 'core', there is no longer a 1-1 relationship between modules and hooks.

So if module 'foo' implements 3 services and a .module file and all have .bar, how would invoke react?

- a) call just the legacy hook
- b) call all 4 hooks, but how to merge - then?
- c) have a separate additional namespace for modules, like module/service

That means in essence that indeed we need to deprecate getImplementations() or at least allow it to return modules that are no modules, but pseudo modules and do the same for invoke.

getCallables() / invokeAll() however would work and continue to work - even if the concept of modules is removed / changed.

I personally would be okay to allow 'module/service' for invoke() - that would be a pure extension and maybe allow getImplementations() to return ['module', 'module/service'] as well - but I am not sure if that is BC breaking.

hmmm

Other way:

Deprecate getImplementations() / invoke() replace with something else. Class hooks would just not work with those legacy concepts. Feels bad, too.

hmmm

I think even if 'module/service' or 'module_hash' is ugly, I think it is better than having class hooks work just half the time - however that would also break the $function = 'module_' . $hook pattern.

Deprecate getImplementations() / invoke() replace with something else. Class hooks would just not work with those legacy concepts. Feels bad, too.

I think we can probably consider getImplementations() + $function = $module . '_foo()'; not part of the public API/@internal - we just say we'll return a list of modules implementing the hook, not what you can do with that list afterwards.

Would need a very clear change notice/deprecation and would probably help to grep for affected contrib modules, but still seems doable in a minor to me.

invoke() is harder. I see search and views using it in core via a quick grep. That should have been deprecated years ago. Also people mis-use that to call procedural functions that aren't hooks instead of doing module_exists() first. We should add an alternative, convert the core usages, see what might break in contrib then try to main bc for those cases if we can, and @deprecate so that people stop using it.

One way around the multiple hook implementations in a module problem would be to actively prevent that for the span of a minor release, maybe.

RDF a new method to handle this seems OK.

It turns out ModuleHandler::invokeAll() unlike module_invoke_all already can be used for altering without any API changes:

https://3v4l.org/H3FsW

Though we would need to add a count($x) + switch for up to 3 arguments, which can be considered a bug fix as if I pass a reference in the array it should work to call a function. In that regard call_user_func_array() is even broken - as that should be supported if an array index already _is_ a reference. Also because the hack to make all args references works (https://3v4l.org/A2oSh). Will try to discuss with PHP devs.

That means we can start converting away from $function() now and use moduleHandler->invokeAll() exclusively.

Plan of action for 8.1.0 would be:

- Consider getImplementations() + $function() @internal
- Deprecate getImplementations()
- Deprecate invoke()

- Add invokeAllReturnList(), which does not do the array_merge_recursive() - which is the main thing missing then.
- Consider to maybe add a new method, which works like alter(), but does not need the &$reference part of invokeAll - perhaps call it mutateAll() or such.

Lots of blocked / postponed issues point to this one.
Would be useful to have an update.

This wants something like functional CPS.

Patch flying in that implements ModuleHandler::invokeByClosure and ModuleHandler::invokeAllByClosure with some example usages.

If this works and we agree on this path, rewriting another dozen calls of ModuleHandler::getImplementations is straightforward.
(The only nontrivial i've seen is in Theme\Registry::build)

Once all calls are done by ModuleHandler, we can smoothly eventify hooks.

Fixed typo.

OK, removing HookDiscovery and ViewsData examples, for now it's just about a POC.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -383,31 +384,52 @@ public function implementsHook($module, $hook) {
   +   */
   +  public function invoke($module, $hook, array $args = []) {
   +    $closure = function (Callable $hook_implementation) use ($args) {
   +      return call_user_func_array($hook_implementation, $args);
   +    };
   +    return $this->invokeByClosure($module, $hook, $closure);
   +  }
   

   We should figure out whether this is a performance regression ... we could keep the old implementation around. Copying code is less bad than people think it is.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   --- a/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   
   +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   @@ -208,6 +210,43 @@ public function resetImplementations();
   +  public function invokeByClosure($module, $hook, Callable $closure);
   

   Adding new methods to those interfaces are tricky.

>We should figure out whether this is a performance regression ... we could keep the old implementation around. Copying code is less bad than people think it is.

Yes, elegancy points don't give performance. So see it as educative comment.

> Adding new methods to those interfaces are tricky.

Can you explain? It's a pure API addition. And it is imho the only sane way to solve this issue.

We're OK to add a new method to ModuleHandler/ModuleHandlerInterface because it's a 1-1 class/interface relationship.

The patch looks pretty good to me - we could leave the old ModuleHandler::invoke() method alone and look at refactoring that to use invokeByClosure() in a follow-up though.

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -311,10 +311,11 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
+          Callable $hook_implementation, $module) use (&$settings, &$assets

Nitpick: 'callable' typehint should be lowercase, I think, to match other built-in types such as 'int' and 'bool'.

(Also in other parts of the patch too.)

The patch looks pretty good to me - we could leave the old ModuleHandler::invoke() method alone and look at refactoring that to use invokeByClosure() in a follow-up though.

As written above, I do agree. This way we can make actual easy progress here.

Re-roll (review coming soon).

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -311,10 +311,11 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
   +          Callable $hook_implementation, $module) use (&$settings, &$assets
   

   I assume this callable type hint is why ModuleHandlerInterface::invoke*() passes on the concatenated module and hook names as $hook_implementation? If we decide to split up that parameter, we can replace this type hint with assert(function_exists($module . '_' . $hook));. Alternatively we can introduce a legacy hook factory (class or method) that simply wraps any function(callable $hook_implementation) in a function(string $hook, string $implementation)

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -384,31 +385,52 @@ public function implementsHook($module, $hook) {
   +    // @todo Use (or emulate) and cache Closure::fromCallable.
   

   I read the docs, but Closure::fromCallable()'s purpose hear is not immediately clear to me. Could someone shed some light on that? :)

3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   @@ -207,6 +209,43 @@ public function resetImplementations();
   +   *   - Callable $hook_implementation
   +   *     The hook implementation to call.
   

   Can we split this into $hook and $module, and do the same for invokeAllByClosure? That way the closure signatures are identical and the additional invokeByClosure(..., $module) parameter could be helpful for abstract/reusable closures.

4. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   @@ -207,6 +209,43 @@ public function resetImplementations();
   +  public function invokeByClosure($module, $hook, Callable $closure);
   ...
   +  public function invokeAllByClosure($hook, Callable $closure);
   

   These methods technically don't take closures, but callables. What do you think of renaming them to ::invokeWith() and invokeAllWith()?

This implements 1, 3, and 4 from #34. What do you think? :)

This attempts to fix code style violations (odd that they cause builds to fail now, but fixing them is far from straightforward?), and moves ModuleHandler::buildFunctionInvoker() to FunctionInvoker so it can be used elsewhere. The intention is that FunctionInvoker is publicly available as a convenience wrapper for any invokers of function-based hook implementations, and code such as building function names ($hook_implementatoin = $module . '_' . $hook;) lives here rather than in the module handler, which should not prefer one type of hook invocation over another.

This fixes the code style violations ((cd core && curl $CODESNIFFER_FIXES_PATCH_URL | patch -p0 -u) did the trick).

Hah. The original patch file had a typo in its extension, but there were no validation errors when trying to upload it.

Quite a few bits of code used to call ModuleHandlerInterface::invoke() inside a loop over ::getImplementations(). The call to ::invoke() (which abstracted out the actual calls to hook implementation functions) is now (often) replaced by $hook_implementation() inside the closures passed on to ::invoke*With(). From a testing point-of-view, calling ::invoke() also meant we could mock that call and prevent any procedural functions calls from taking place during unit tests.

TLDR; we shifted the responsibility of directly invoking hook implementations from the module handler to calling code. Can we still write unit tests for calling code invoking procedural functions, or should those become kernel tests?

This converts a few more calls to ::getImplementations() (the ones showing up in Drupal CI's failures, but there are many more). It also converts two calls without updating the tests, because of the problem described in #49.

Hah. The previous patch exposed workspace_entity_load() erroneously taking its first argument by reference, causing the majority of test failures. This patch fixes that as well as some code style violations.

This 'fixes' one Workspaces-related test failure by making the hook_entity_load() documentation match reality. Also see #52. This leaves two test failures caused by tests running invokers that call procedural functions. See #39 for more on that. There are also plenty of other calls to ModuleHandlerInterface::getImplementations() that don't trigger test failures, either because the current two failures hide them, or because of lack of coverage.

@Xano: wow, you really got a lot of this forward. I like the idea of a dedicated caller object that caries metadata.

Alas, i wondered if this is overly complex. On further looking, i found that the overly complex part in fact came from the original author (erm, me....).

So now i think i was on the wrong track in #24.

#24 moves the implementation details of hook implementation from the invocation like this (but trades in some js-like callback hell):

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -311,10 +311,11 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
-        foreach ($this->moduleHandler->getImplementations('js_settings_build') as $module) {
-          $function = $module . '_' . 'js_settings_build';
-          $function($settings, $assets);
-        }
+        $this->moduleHandler->invokeAllByClosure('js_settings_build', function (
+          Callable $hook_implementation, $module) use (&$settings, &$assets
+        ) {
+          $hook_implementation($settings, $assets);
+        });
       }

What if we do it in a much more simpler way like this:

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -311,10 +311,11 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
-        foreach ($this->moduleHandler->getImplementations('js_settings_build') as $module) {
-          $function = $module . '_' . 'js_settings_build';
-          $function($settings, $assets);
-        }
+       foreach ($this->moduleHandler->getImplementationCallers('js_settings_build') as $function) {
+        $function($settings, $assets);
+      }

For now just some quick notes so if we want to go this simpler way i'm not feeling too guilty as of wasted work... ;-)

* $function in that code should nevertheless be a __invoke-able object with some metadata
* We can then make hook implementation collectors as tagged service
* As of the question of Closure::fromCallback: See the RFC, it is said that converting a callback (function name, class/method array) to a closure, statically caching it and using this multiple times has significant performance gains. We can leave that to a followup though.

What do you think? Or am i on the wrong track now and have just forgotten something i was aware of in #24?

Thanks for sharing your thoughts! I'm not too worried about adding one layer of indirection to an API we want to make more abstract. The advantage of moving from just closures to any callable is that, indeed, any invoker or decorator can be either a function or an object that implements ::__invoke(). In essence, we could as well introduce an InvokerInterface with a single method, but callables work just as well, minus the type checks.

Hmm, sleeping over it, i think i remember why i added that indirection in the first place: A major obstacle with this is to pass parameters by reference, where PHP has some limits (i think it was in call_user_func_array), which may or may not be fixable via PHP5.9 variadic parameters (unfortunately D8 requires PHP5.5.9) or closures. In any case, my forgetting the exact interrelationship of this all is a strong hint we should document this once groked.

Drupal 8.7 will require PHP 7

> The previous patch exposed workspace_entity_load() erroneously taking its first argument by reference, causing the majority of test failures

> This 'fixes' one Workspaces-related test failure by making the hook_entity_load() documentation match reality.

Those should be fixed in separate issues.

(And the second fix looks wrong to me anyway -- the items in the $entities array are objects, so no need to pass the array by reference.)

(And the second fix looks wrong to me anyway -- the items in the $entities array are objects, so no need to pass the array by reference.)

It took me a bit to realize what was going on: it's not the entity objects themselves that are touched, but the array values that are replaced. This requires an array reference.

Hmm, sleeping over it, i think i remember why i added that indirection in the first place: A major obstacle with this is to pass parameters by reference, where PHP has some limits

That's a good point! The real problem here is call_user_func*() being incapable of handling references. Any of the other solutions (closures' scope inheritance, variadics, custom classes) do not have this problem, but they also allow calling code to dictate the actual hook invocation, which is the abstraction we need to support different types of hook implementations (functions, services, plugins, whatever you desire!) although we can discuss if this is the right place for it.

Drupal 8.7 will require PHP 7

Does that mean we can use funky goodies like scalar and return type hints in this patch? :D

> It took me a bit to realize what was going on: it's not the entity objects themselves that are touched, but the array values that are replaced. This requires an array reference.

That sounds weird, and possibly a misuse of the hook by that implementation, and a rabbithole for this issue -- let's figure out in a different one.

Re: The php 7 funkiness ;-) From the discussion issue (https://www.drupal.org/project/drupal/issues/2842431)

All currently supported PHP versions will be tested during the 8.7.x development phase priorto 8.7.0, since PHP 5.5 must be supported for Drupal 8.6 backports.

So not yet, from the start of the 8.8 branch we'll be able to use them.

Ah, so PHP7 only code can be used for 8.7.1 but not 8.7.0, interesting. Well, then I guess it depends on how urgently you want this issue done!

Now i remember: The problem was func_get_args() (see here) and my energy goes with using PHP-5.6/7 variadic arguments and aiming for Drupal 8.7.1.

This should fix the remaining two failing test cases.

@axel.rutz Thanks for sharing that memory! I see how func_get_args() has trouble with references. Do you remember where you ran into this being a problem for this issue?

Ah, ViewsDataTest. I think we'll need to move that to a kernel test too, like HookDiscoveryTest. Any help on that (Views) will be appreciated :)

// Edit: The views_test_data module was helpful, and most of the tests relate to caching, so I started work on converting this test :)

This patch fixes ViewsDataTest by converting it to KernelTestBase. As a consequence, my local commit looked liked 2 files changed, 216 insertions(+), 295 deletions(-) (yay!).

This fixes ViewsHookTest and applies the style fixes Drupal CI generated.

Need CR to be added to code as well

+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
@@ -175,6 +175,10 @@ public function getHookInfo();
+   * @deprecated Will be removed before 9.0.0. Use the self::invoke*() methods
+   *   instead. To pass arguments by reference or process return values, use
+   *   self::invoke*With*().

Needs proper deprecation according https://www.drupal.org/core/deprecation#how-method

I added a change record placeholder (considering the complexity I decided to wait with writing it until we agree on the definitve approach), and added a deprecation error that includes the change record's URL. This should break some tests! :)

66 more calls to ModuleHandlerInterface (and their tests) remain. If any of those tests are built on UnitTestCase, converting them may mean the patch multiplies in size (see ViewsDataTest's conversion in the patch). Is that okay for this issue? :)

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -966,7 +966,7 @@ function hook_ENTITY_TYPE_revision_create(Drupal\Core\Entity\EntityInterface $ne
- * @param \Drupal\Core\Entity\EntityInterface[] $entities
+ * @param \Drupal\Core\Entity\EntityInterface[] &$entities
  *   The entities keyed by entity ID.
  * @param string $entity_type_id
  *   The type of entities being loaded (i.e. node, user, comment).
@@ -974,7 +974,7 @@ function hook_ENTITY_TYPE_revision_create(Drupal\Core\Entity\EntityInterface $ne

@@ -974,7 +974,7 @@ function hook_ENTITY_TYPE_revision_create(Drupal\Core\Entity\EntityInterface $ne
  * @ingroup entity_crud
  * @see hook_ENTITY_TYPE_load()
  */
-function hook_entity_load(array $entities, $entity_type_id) {
+function hook_entity_load(array &$entities, $entity_type_id) {

This issue is not the place to go changing the definition of hooks, whether that change is considered a bugfix or an API addition. Please could someone file a separate issue for that.

@Xano for deprecations that require a lot of changes, in the past the message has been added to \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations()

This issue is not the place to go changing the definition of hooks, whether that change is considered a bugfix or an API addition. Please could someone file a separate issue for that.

I wouldn't be against that, but I wonder if that would take away from the understandability of this patch, since I updated the hook documentation after I discovered that the hook in reality did not work as documented.

> since I updated the hook documentation after I discovered that the hook in reality did not work as documented

The hook works as documented. workspace module is misusing it. hook_entity_load() is not meant to allow you to swap entities in and out of the array.

We can either a) fix workspace module, or b) enhance the API.

Whichever one we do, it needs its own issue to discuss it, not be tacked on here. This issue, unfortunately, will need to be postponed for that -- or we could leave hook_entity_load() out of this issue and file a follow-up for it, so the majority of the work done here can get in.

Thanks for the feedback! I opened #3001101: workspaces_entity_load() violates the hook interface (a.k.a. introduce hook_entity_load_alter() and hook_ENTITY_TYPE_load_alter()) for the Workspaces bug.

This reverts the changes related to hook_entity_load() in favor of #3001101: workspaces_entity_load() violates the hook interface (a.k.a. introduce hook_entity_load_alter() and hook_ENTITY_TYPE_load_alter()), and ignores the newly introduced deprecation. I tried running a test that ends up calling ModuleHandlerInterface::getImplementations(), and it seems the deprecation error message is somehow not collected and filtered.

I cannot reproduce the failures locally, so triggering a re-test.

Weird. I cannot reproduce this locally. Does Drupal CI use a non-default environment for functional JS tests?

Don't know about the failures but I do have some thoughts.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -311,10 +312,13 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
   +          callable $hook_implementation) use (
   +&$settings,
   + &$assets
   +        ) {
   

   This indentation is whack and shows up a lot in the patch. Does this really comply with our standards? Did the automated patch go nuts or something?

   At the very least this should still be standards compliant and a lot more readable:

           $this->moduleHandler->invokeAllWith('js_settings_build', new FunctionInvoker(
             function (callable $hook_implementation) use (&$settings, &$assets) {
               $hook_implementation($settings, $assets);
             }));
   

I have lots of questions about the FunctionInvoker it seems.

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -214,8 +215,12 @@ protected function invokeCronHandlers() {
   +    $this->moduleHandler->invokeAllWith('cron', new FunctionInvoker(function (
   

   Why do all the callers have to inject the module hook invoking logic? Why doesn't invokeAllWith take care of that logic for the caller and can't ModuleHandler be opinionated about how this is suppose to work right?

2. diff --git a/core/lib/Drupal/Core/Extension/Hook/FunctionInvoker.php b/core/lib/Drupal/Core/Extension/Hook/FunctionInvoker.php
   new file mode 100644
   index 0000000000..f2fe93b9df
   --- /dev/null
   +++ b/core/lib/Drupal/Core/Extension/Hook/FunctionInvoker.php
   

   I'm not sure I'm sold on this class.
   1. What are we actually sharing(or expecting to share) this logic with?
   2. The goal of the ModuleHandler class is largely to encapsulate this logic already right?
   3. Isn't the point of this patch largely to fill a hole so this logic doesn't need to be shared outside of ModuleHandler?

FunctionInvoker's primary goal is to extract the invocation of hooks as we know them now from ModuleHandler and to do so in a self-documenting way. Hence the separate class rather than additional one-line closures everywhere.

Removing invocation-specific logic from ModuleHandler means calling code is free to introduce its own invocations, allowing developers to experiment with hook implementations that allow dependency injection, for instance.

After fiddling with #3001190: Deprecate ModuleHandlerInterface::*Deprecated() in favor of hook metadata I am wondering if we shouldn't move the burden of providing the invoker in hook_hook_info(), instead of providing it call-time, meaning you have to ensure all invocations of the same hook (if more than one) provide the exact same invoker.

Removing invocation-specific logic from ModuleHandler means calling code is free to introduce its own invocations, allowing developers to experiment with hook implementations that allow dependency injection, for instance.

That might be cool I guess. I think in the context of this issue we just solve the problem at hand and though and let ModuleHandler remain opinionated about hooks. We can follow up with this feature and in the mean time if you want events that support more complicated OO models, we've got actual Events.

Here is why. We've strayed pretty far from the goal of this issue and decreased usability by introducing this new feature. The develop can't just pass a callable anymore but. They have to not only instantiate the invoker but the feature you're suggesting is actually a bigger problem I hadn't realized. The _calling_ method has to to know what invoker every hook needs to complete. If there _are_ additional "invokers" in the future hooks start to loose the on simple promise of an interface they provide and callers are going to start running into a mess of compatibility issues that aren't really solvable because functions do not have interfaces the only have a method signature.

The develop can't just pass a callable anymore but

They still can. In fact, that's all FunctionInvoker needs to be in order to pass the type checks.

They have to not only instantiate the invoker but the feature you're suggesting is actually a bigger problem I hadn't realized. The _calling_ method has to to know what invoker every hook needs to complete.

Yup. Totally. Hence my note about putting this in hook_hook_info() since that's all we have to declare hook metadata.

If there _are_ additional "invokers" in the future hooks start to loose the on simple promise of an interface they provide and callers are going to start running into a mess of compatibility issues that aren't really solvable because functions do not have interfaces the only have a method signature.

I don't quite understand this. The hook system hardly has or had enforceable interfaces (with the exception of the generic API components and hook implementations' parameters). It is and always has been very easily possible to invoke or implement hooks with incorrect signatures, and then getting somewhat non-descriptive stack traces.

If we want the cleanest approach, maybe we should won't-fix this and tell folks to stick with legacy hooks, or use events or plugins instead? Even if we come up with an invoker that lets developers use DI for their hook implementations, modules can still only implement each hook once, as opposed to providing multiple plugins of the same type, or registering multiple listeners for the same event. The original report said some code may want to trigger events when hooks are invoked by swapping ModuleHandler, but unless that's for debugging purposes, perhaps that's better done by having some wrapping code invoke the hook and the event, for instance.

Just a note that when things like this are proposed/added/changed, the API module maintainers need to know about it, because the API module does some special parsing/recognition for hook invocations and hook implementations. I'm now following this issue...

Re-roll for 9.1.x-dev.

Adding core/lib/Drupal/Core/Extension/Hook/FunctionInvoker.php file , and solving coding standards error.

Re-roll for D9.1.x based on the patch in #84.

Leaving as NW because this still needs to handle the replacement of assertArraySubset().

Rerolled 18 months ahead to get an MR and a fresh perspective, and other improvements. It seems like we're a long way to go here.

I'm all for closures, etc. But something seems wrong with the approach. Its very.. weird. Unenforced signatures is just a minor aspect, calling closures or functions from closures feels a bit whack when its running in the debugger. Might need some time for this to sink in.

There are quite a few instances of count(mh->getImplementations) > 0 or similar in the codebase. It seems like in order to satisfy these cases without complicating things with closures it would make sense to introduce a hasImplementations method.

Not a fan of unit -> kernel conversions from #49 #76.

Agree with #89 re indentation of closures.

Changes

• More progress on test fixes from previous conversions, introduced a ClosureInvoker which needs to be dealt with, but allows us to mock implementations in unit tests without actually needing to implement procedural hooks or do includes like a bunch of current tests do. This also means I've been able to revert the unit->kernel conversion, then modifying very little LOCs.
• Addressed new linting and other failures exposed by CI.
• De-uglified closures (multiline args), for better or worse.

--

Perhaps these invoker calls can instead be assigned to local variables to improve readability? Rather than uglifying parameter list to one param per line.

Given the new closures are all new scopes, perhaps all definitions of $hook_implementation should be camelCased?

This should pass, except for a Composer 2.2 related assertion being fixed over at #3255749: Composer v2.2 prompts to authorize plugins

Took this issue all the way to implementing all ~60 usages of getImplementations in core.

Went along with the \Closure::fromCallable suggestion and reworked the patch for simplicity. Removed FunctionInvoker, since we're not aiming to do some kind of class based event-thing here; bit of friction above.

Padded out the change record with rationale, examples, and a list of changed invocations in core.

I suggest reading the change record before code review.

Code changes

• Renamed $invoker to $callback, similar to PHP's builtin array_* functions and their language. This reduces confusion with the similarly named hook invoker closure ($hookInvoker).
• Removed FunctionInvoker in favor of \Closure::fromCallable, agree with #91. \Closure::fromCallable solves a bunch of discussions about FunctionInvoker above. I’m keen to get some feedback considering how much cleaner it appears.
• Updated invokeAllWith docs so it matches PHPStans documentation for closures. https://phpstan.org/writing-php-code/phpdoc-types#callables
• Modified all usages of moduleImplements+invoke to use invokeAllWith
• Fixed value of invokeAllWith AND documentation
• Introduced hasImplementations, since there's a few instances in core where we only care there are implementations. Interface is already changing.

Notes

• There is only one deprecation trigger left. The unit test covering getImplementations itself.
• An interesting side effect of delegating invocation to module handler service is there could potentially be more than one hook implementation per module, if the implementation wants to go out and invent its own way of doing things.
• There are numerous tests that no longer touched getImplementations (usually combined with $this->any()). So those lines were removed without replacement.
• Used \Closure::fromCallable, PHP 8.1 has first class callables like \mymodule_myhook(...). Currently using call_user_func_array in legacy invokeAll/invokeAll, at least until we can use spread with PHP 7.4 in Drupal 10. I've run tests in previous test runs which show spread works just fine. PHP RFC for those interested.

I think this is vastly easier to understand compared to previous iterations. Hook implementations are not transparent to the caller, other than references to the module name, to retain backwards compatibility. And most importantly parameters may be mutated. \Closure::fromCallable is quite elegant, and it looks even better with PHP 8.1 syntax.

Looks really good to me, left some very minor comments on the MR.

MR looks great! Could use another review as I skimmed only

Fixed a minor thing in the change record: under 'Old implementation', 'hook' should be '$hook'.

Awesome to see this nearly done; good job everyone!

@neclimdul @catch variable name updated to suit CS.

Both the issue summary and the change record need to be updated to detail how to cope with usages of getImplementations() like the one on http://grep.xnddx.ru/node/32014765 - where it is used to build a list of modules that implement a hook to get info to the user.

It does seem odd that we're removing the ability to get a list of modules that implement a specific hook. Looking at the event dispatcher for something similar - it is totally possible to get all the listeners for a specific event - see \Symfony\Component\EventDispatcher\EventDispatcherInterface::getListeners(). Are we sure that we want to remove this capability?

Maybe we want some other method on the module handler that only lists modules for a specific hook? I think we need to look at http://grep.xnddx.ru/search?text=getImplementations&filename= and see how getImplementations() is currently being used. There are a number of use-cases that are not "trigger this hook" - http://grep.xnddx.ru/node/30840575#line-36 is another example.

where it is used to build a list of modules that implement a hook to get info to the user.

It does seem odd that we're removing the ability to get a list of modules that implement a specific hook.

@alexpott it is possible to get a list of implementors of a hook. Keep in mind that the callback method gets both the $hook closure and the module machine name.

Its as simple as:

$modules = [];
\Drupal::moduleHandler()->invokeAllWith('thehook', function (callable $hook, string $module) use (&$modules) {
  $modules[] = $module;
});

It is a tiny bit of work to do this, but I think that's the point of this issue. To make it more difficult to figure out the implementors and call functions manually.

There are 4x examples in the MR, see:

• HelpTest.php 1x
• node.module 1x
• ModuleHandlerTest.php 2x

Maybe we want some other method on the module handler that only lists modules for a specific hook?

This would basically bring back getImplementations. Doesnt seem desirable.

Both the issue summary and the change record need to be updated to detail how to cope with usages of getImplementations() [..] where it is used to build a list of modules that implement a hook to get info to the user.

Added example to the change record.

Updated MailManager's single call to an implementation hook_mail.

Updated summary per #113

@dpi I'm not sure I agree with the point of the issue being

To make it more difficult to figure out the implementors and call functions manually.

I think the point of the issue is to make it unnecessary to use getImplementations() to call the functions manually and to make how hooks are invoked an internal implementation of the module handler.

Thanks for pointing out the use cases in core that we still have. Looking at the node.module example as the one real runtime usecase I think this argues for a new method on the moduleHandler. Oh... lol this method already exists... \Drupal\Core\Extension\ModuleHandlerInterface::implementsHook()... I think that we should convert the node.module to use that... Or we could consider improving the new ::hasImplementations(). If we change the signature of hasImplementations($hook) to hasImplementations($hook, string|array|null $module) - and if $module is present limit it to discovering if the provided module or modules implement the hook. We could then also deprecate ::implementsHook()

Thanks @alexpott,

Appreciate the comms here and DSlack. Taken your feedback in.

• I've deprecated implementsHook and switched over existing calls to hasImplementations.
• hasImplementations signature improved to take in single or multiple modules.
• Node and tests has been switched over to use the revised hasImplementations

Unfortunately had to rebase, Gitlab UI was being sticky about it despite merges to 9.4.x, so I succumbed.

Back to needs review.

The MR looks good to me.
I open a couple of threads on the MR.

Is there deprecation test needed? or all usage is removed?

Feedback from @daffie @alexpott addressed.

The Mr looks great, almost RTBC. Just a couple of nitpicks.

Majority addressed, pushed back on one.

All code changes look good to me.
All my remarks are addressed.
For me it is RTBC.

Remaining question here for me is that it's not 100% clear when to use ::invokeAll() vs. ::invokeAllWith(), also think that hooks like hook_rebuild(), hook_file_download() and probably mean more could move to ::invokeAllWith() if we want to - basically anything that doesn't need the array merging behaviour, which is probably most of the hooks we have left.

However, currently there isn't any choice at all, so that feels like follow-up material.

For #128 I think that that suggests somehow indicating to the system that that return value of all hook_rebuild() implementations is void and therefore the array munging of invokeAll is unnecessary. It'd be nice to somehow use PHP attributes to denote this - but we're getting into event/hook registration territory so definitely feels best resolved in a follow-up.

@catch @alexpott created #3266896: Switch usages of invokeAll to invokeAllWith if result of merge is unused as a follow up task to switch over usages of invokeAll that don't make use of return values.

We're going to need a 10.0.x version of the MR as well as we need to commit this to both branches.

Drupal 10 edition of the PR created at MR1902.

A simple rebase, resolving the following conflicts:

• #3261239: Remove deprecations from search module — Removed modified lines; no longer exists.
• #3097889: Remove deprecated theme functions — Removed modified lines; file no longer exists.

We also should open an issue to refactor drupal_check_module(). Pre-install checks should be done using composer rather calling code from the module. Running code from a module prior to its installation is fraught. The autoloader does not work at all as one would expect. Interesting. But yeah that's not something we can fix here so very worth documenting.

I assume its known the requirements hook w ($phase == 'install') invoked by \drupal_check_module() cannot call its own code during this time. Is there a way of working around this with Composer? Sure, if dependencies were being checked here then it should be removed. But runtime only checks..? Are we proposing removing this functionality without replacement?

I assume its known the requirements hook w ($phase == 'install') invoked by \drupal_check_module() cannot call its own code during this time

I think this is a known issue. With the current way the autoloader is manipulated by the DrupalKernel there's no way for a module to fix this. The only fix would be to change Drupal's approach to autoloading modules and have them part of the autoloader generated by Composer but this will break all sorts of things - there are places that do class_exists() checks to determine if something is installed (which is a bad idea but it still happens). This a gnarly knot and yeah I think the long term aim would be to make install phase checks only use things that can be defined in a composer.json and all other checks move to runtime or update. The trickiest place for this would system_requirements - but I think we could special case that one - in some way.

Yep this is covered in the docs of hook_requirements...

 * During installation (when $phase == 'install'), if you need to load a class
 * from your module, you'll need to include the class file directly.

:D lovely.

Discussed with @catch and @dpi again. The follow-ups are in place - this is looking good. Nice work work @dpi.

Great to see this one ready after 7 years. Hopefully we can keep going on #2237831: Allow module services to specify hooks now too.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Published the CR and unpostponed the follow-up.

Committed e4579e1 and pushed to 9.4.x. Thanks!

Reverted the commit from 9.4.x because it was the 10.x patch and applied and committed the 9.4.x patch.

Thanks everyone for sticking with it. I appreciate the earlier generations of work, and fast and frequent MR reviews.

Note:

As of the question of Closure::fromCallback: See the RFC, it is said that converting a callback (function name, class/method array) to a closure, statically caching it and using this multiple times has significant performance gains. We can leave that to a followup though.

The important part here is "statically caching it and using this multiple times".
But this is exactly NOT what we are doing.
A new closure is created in every iteration and every call, and never cached.
And even if we would cache it, it would only pay off if the same hook is called multiple times in the same request. So we would have to know how many times a hook is called on average.

Now we cannot really go back, because there could be contrib or custom modules out there that call ->invokeAllWith with a callback argument that expects a Closure instead of callable.

// this would break if we pass a callable-string.
$mh->invokeAllWith('some_hook', static function (\Closure $impl) {..});
// this will work if we pass a callable-string.
$mh->invokeAllWith('some_hook', static function (callable $impl) {..});

Re #146 let's continue optimizations in one of following issues
- #329012-107: Remove call_user_func_array() from ModuleHandler::invoke() + invokeAll()
- #3259716: Replace usages of static::class . '::methodName' to first-class callable syntax static::method(...)