Delegate all hook invocations to ModuleHandler

Created on 17 November 2015, about 9 years ago
Updated 12 June 2023, over 1 year ago

Problem/Motivation

Pretty much all of the other issues that are asking for OO hook implementations all run into the same problem: we have an unsettlingly large number of places throughout core that build a function name and then call it with some arguments, rather than calling ModuleHandler->invoke() or ModuleHandler->invokeAll().

This actively prevents implementation of a contrib module that, as an example, extends ModuleHandler and emits events when hooks are invoked. Pretty much anywhere where ModuleHandlerInterface->getImplementations() is used outside of ModuleHandler, there's likely a function name being constructed, and in the vast majority of these cases, it's unnecessary.

Proposed resolution

  • Introduce two new hook invocation methods that take an additional callable (often, but not limited to, closures) that takes care of calling each individual hook implementation. Code currently calling ModuleHandlerInterface::getImplementations() to invoke hooks themselves can use custom callables to customize their hook invocations. This is useful to pass on extra parameters or process return values.
  • Mark ModuleHandlerInterface::getImplementations() deprecated, to encourage developers to use the (new) ::invoke*() methods instead.
  • Code that genuinely needs to know the implementors of a hook may continue to do so, by making use of of the $module parameter of invokeAllWith. See the change record.

Remaining tasks

Review, evaluate DX.

User interface changes

None.

API changes

None. Existing APIs are merely extended. ModuleHandlerInterface is extended, which is not considered a break (@catch in #29)

Invoking modules must now replace their code which builds functions with calls to Module Handler. The call simply includes the hook name, and a closure which in turn calls the original hook via a closure. Modules that originally dealt with the return values of the invoked hooks, or relied on mutation of hook arguments, should pass along state via a use definition in the Module Handler closure.

Data model changes

None.

πŸ“Œ Task
Status

Fixed

Version

9.4

Component
BaseΒ  β†’

Last updated about 5 hours ago

Created by

πŸ‡ΊπŸ‡ΈUnited States cweagans Boise, ID, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡©πŸ‡ͺGermany donquixote

    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) {..});
    
Production build 0.71.5 2024