Improve HookRegistry hook calling conventions

[asmecher]

HookRegistry has some long-standing implementation quirks that make hook-based code hard to read and maintain:

• Hook parameters are be passed through an array and must be unpacked in sequence
• Type hinting is not possible within the parameter array
• Named parameters are not available within the parameter array
• The return value from a hook callback is perpetually confusing

Clean up these conventions (in a backwards-compatible way).

Proposal PR: #8084

Passing parameters to hook callbacks

The proposal deprecates HookRegistry::call and adds a new HookRegistry::execute to be preferred in its place.

HookRegistry::execute offers several new possibilities for callbacks:

Callback function parameters properly declared

Previously, callbacks had to pass parameters via an array:

HookRegistry::register('someHookName', function(string $hookName, array $args) {
    list($submission, $publication) = $args;
    ...
});

HookRegistry::call('someHookName', [$mySubmission, $myPublication]);

Using HookRegistry::execute instead, the parameters can be named directly, including type hinting:

HookRegistry::register('someHookName', function(string $hookName, Submission $submission, Publication $publication) {
    ...
});

HookRegistry::execute('someHookName', [$mySubmission, $myPublication]);

References are also supported; these can be declared in the callback parameter list as usual, and should be passed into the array as references as well (as before, even with objects):

HookRegistry::register('someHookName', function(string $hookName, Submission $submission, Publication &$publication) {
    ...
    $publication = $someOtherPublication;
});

HookRegistry::execute('someHookName', [$mySubmission, &$myPublication]);

Named arguments

HookRegistry::execute supports named arguments; this can help keep code readable when longer lists of parameters are provided or parameter ordering might be unclear.

HookRegistry::register('someHookName', function(string $hookName, Submission $submission, Publication $publication) {
    ...
});

HookRegistry::execute('someHookName', ['submission' => $mySubmission, 'publication' => $myPublication]);

Hook return conventions

In the past, hook callbacks returned true to interrupt later hooks, or false to continue. The meaning of these values was hard to remember. This change adds HookRegistry::HOOK_ABORT and HookRegistry::HOOK_CONTINUE constants to use instead. (This change is backwards compatible; the constants are equivalent to true and false.)

HookRegistry::register('someHookName', function(string $hookName) {
    return HookRegistry::HOOK_ABORT; // Stop processing subsequent hook registrants
});

[NateWr]

I like it! 👍 A few thoughts:

1. If we're deprecating and renaming things, what about shortening it slightly to Hook instead of HookRegistry. (HookRegistry can still be around for backwards compatibillity.)

2. Instead of register and execute, what about Hook::add() and Hook::run()?

3. Is aborting a hook necessary? I never could think of a good use case for this. I wonder if it's worth thinking about an approach where the result of a callback can be passed to the next callback. Something like:

HookRegistry::register('my::hook', function(string $hookName, Submission $submission, $result = null) {
    return true;
});
HookRegistry::register('my::hook', function(string $hookName, Submission $submission, $result = null) {
    if ($result === true) {
        return $result;
    }
    return false;
});

Not loving that syntax, but you get the idea. A bit like array_reduce() or similar methods where the result accumulates over each iteration. (This is, for example, how middleware is implemented in some routers. Each middleware receives the request and response, and modifies the response, then the next middleware receives the modified response.) Maybe a special kind of hook could be used?

5. Maybe don't repeat "hook" in the constant names? So HookRegistry::HOOK_SEQUENCE_CORE could be HookRegistry::SEQUENCE_CORE.

6. The last big question for me is around documentation and discovery of hooks. I think that's been a consistent complaint over the years. How do people know what hooks are available and what parameters they receive? A couple of ideas:

We could use small static classes for hooks, which would give us code completion and type hinting in the editors. The downside is that we'd have a lot of small classes added to the codebase:

use PKP\hook\Submission as HookSubmission;
HookRegistry::register(HookSubmission::add, function(Submission $submission) {});
HookRegistry::execute(HookSubmission::add($submission));

Or we could come up with a consistent docblock format. We would be able to generate documentation, but wouldn't get in-editor code completion:

/**
* A submission has been added
*
* @param Submission $submission
*/
Hook::execute('Submission::add', [$submission]);