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

• The HookRegistry class has been renamed to Hook, with backwards-compatible class aliases provided.
• Hook::add is now preferred to HookRegistry::register. Backwards compatible.
• Hook::run is now preferred to HookRegistry::call. Backwards compatible -- but to emulate old calling conventions using a parameter array, wrap the parameters in [] a second time:

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

Passing parameters to hook callbacks

Hook::run 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 Hook::run instead, the parameters can be named directly, including type hinting:

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

Hook::run('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):

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

Hook::run('someHookName', [$mySubmission, &$myPublication]);

Named arguments

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

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

Hook::run('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 Hook::ABORT and Hook::CONTINUE constants to use instead. (This change is backwards compatible; the constants are equivalent to true and false.)

Hook::add('someHookName', function(string $hookName) {
    return 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]);

[nils-stefan-weiher]

Looks good to me!

[asmecher]

There's a tool and a list of alternatives for WordPress hooks that would make a great starting point: https://github.com/pronamic/wp-documentor#alternatives

[asmecher]

Step 1: Get the php-cs-fixer to add @hook self-documentation to function docblocks:

• pkp-lib: Add hook annotations #9338
• ojs: Add hook annotations ojs#4049
• omp: Add hook annotations omp#1475
• ops: Add hook annotations ops#582
  (The re-linting commit is kept separate from the tooling change.)

To generate @hook self-documentation in PHP classes, use...

• Application: lib/pkp/lib/vendor/bin/php-cs-fixer fix --allow-risky=yes --path-mode=intersection --config=.php-cs-fixer.php --using-cache=no -vvv .
• pkp-lib: ./lib/vendor/bin/php-cs-fixer fix --allow-risky=yes --path-mode=intersection --config=.php-cs-fixer.php --using-cache=no -vvv .

Note that these only add @hook self-documentation where it's missing! It won't correct/re-generate hook self-docs when they already exist.

Annotations look like this:

• @hook API::stats::issues::params [[&$allowedParams, $slimRequest]]
  • The [[ ... ]] indicates a nested array, which is the old Hook::call convention.
  • The & indicates that there's a reference on $allowedParams.
  • The corresponding function to accept the hook might look like:

    function issueStatsParamsHookHandler(string $hookName, array $args): bool
    {
      $allowedParams =& $args[0]; // this was passed by reference and can be modified
      $slimRequest = $args[1];
      ...
      return Hook::CONTINUE; // or Hook::ABORT;
    }

• @hook Some::Other::Hook ['param1' => &$param1, 'param2' => $param2]
  • The [ ... ] is not a nested array, indicating the new Hook::run convention.
  • Named parameters are used. (We don't have to use these, but they're nice for long parameter lists. Otherwise positional parameters will be used instead.)
  • The corresponding function to accept the hook might look like:

    function someOtherHookHandler(string $hookName, string &$param1, string $param2): bool
    {
      ...
      return Hook::CONTINUE; // or Hook::ABORT;
    }

[asmecher]

Step 2: Generate a human-readable hook list.

It turns out there was already a hook listing tool -- lib/pkp/tools/getHooks.php -- that dumped a JSON-encoded bare list of hooks by name.

I've extended that in the PRs above to generate more information, and in multiple formats, including RST (which is supported -- barely -- by PHP's hand-written documentation toolkit).

Result: A list of hooks added to the phpDocumentor output, with a link to it from the sidebar:

This could use improvement, such as links from the filenames into that part of the reference, but the hand-written documentation feature seems very much to be a work in progress.

The normal phpDocumentor output also lists custom tags in context with function self-documentation:

It's got some room to improve, but I think this is ready to launch.

[asmecher]

All merged! We have a hook list that can be generated in each application by...

# Update the hook list in PHPDocumentor's guide
php lib/pkp/tools/getHooks.php -r > docs/dev/guide/hooks.rst

# Re-generate PHPDocumentation
php phpDocumentor.phar

# Documentation will now be available in docs/dev/build

[asmecher]

Just capturing some thoughts in case I decide to push them further. Now that we have first class callable syntax in PHP, we can already use callables for hook registrants. However, we may also consider using first-class callables for hook naming.

Rather than registering and calling hooks as strings...

// Adding a hook registrant
Hook::add('MyClass::myHookName', fn() => {...});

// Running a hook, usually done within the MyClass::myHookName function
Hook::run('MyClass::myHookName');

...we may want to use callables interchangeably with strings:

// Adding a hook registrant
Hook::add(MyClass::myHookName(...), fn() => {...});

// Running a hook, usually done within the MyClass::myHookName function
Hook::run($this->myHookName(...));
// ...or maybe...
Hook::run(__METHOD__); // not actually a first class callable, but also avoids repeating the method name

Experimentally, this can be accomplished with the following:

diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php
index e425759b1b..4103875af2 100644
--- a/classes/plugins/Hook.php
+++ b/classes/plugins/Hook.php
@@ -31,18 +31,19 @@ class Hook
     /**
      * Get the current set of hook registrations.
      *
-     * @param string $hookName Name of hook to optionally return
+     * @param string $hook Hook to optionally return
      *
-     * @return mixed Array of all hooks or just those attached to $hookName, or
-     *   null if nothing has been attached to $hookName
+     * @return mixed Array of all hooks or just those attached to $hook, or
+     *   null if nothing has been attached to $hook
      */
-    public static function &getHooks(?string $hookName = null): ?array
+    public static function &getHooks(callable|string|null $hook = null): ?array
     {
         $hooks = & Registry::get('hooks', true, []);
-        if ($hookName === null) {
+        if ($hook === null) {
             return $hooks;
         }
 
+        $hookName = self::possibleCallableToString($hook);
         if (isset($hooks[$hookName])) {
             return $hooks[$hookName];
         }
@@ -52,23 +53,25 @@ class Hook
     }
 
     /**
-     * Clear hooks registered against the given name.
+     * Clear registrants from the specified hook.
      */
-    public static function clear(string $hookName): void
+    public static function clear(callable|string $hook): void
     {
+        $hookName = self::possibleCallableToString($hook);
         $hooks = & static::getHooks();
         unset($hooks[$hookName]);
     }
 
     /**
-     * Register a hook against the given hook name.
+     * Add a registrant against the given hook.
      *
-     * @param string $hookName Name of hook to register against
+     * @param string $hook Hook to register against
      * @param callable $callback Callback pseudo-type
      * @param int $hookSequence Optional hook sequence specifier SEQUENCE_...
      */
-    public static function add(string $hookName, callable $callback, int $hookSequence = self::SEQUENCE_NORMAL): void
+    public static function add(callable|string $hook, callable $callback, int $hookSequence = self::SEQUENCE_NORMAL): void
     {
+        $hookName = self::possibleCallableToString($hook);
         $hooks = & static::getHooks();
         $hooks[$hookName][$hookSequence][] = $callback;
     }
@@ -114,20 +117,21 @@ class Hook
     }
 
     /**
-     * Call each callback registered against $hookName in sequence.
+     * Call each callback registered against $hook in sequence.
      * The first callback that returns ABORT will interrupt processing and
      * this function will return ABORT; otherwise, all callbacks will be
      * called in sequence and the return value of this call will be
      * CONTINUE.
      *
      * The signature of a callback function should be:
-     *   function callback($hookName, ...) : bool;
+     *   function callback($hook, ...) : bool;
      * where ... corresponds to the parameters named/listed in the $args
      * parameter to Hook::call. These may be named if desired,
      * and may include references.
      */
-    public static function run(string $hookName, $args): bool
+    public static function run(callable|string $hook, $args): bool
     {
+        $hookName = self::possibleCallableToString($hook);
         $hooks = & static::getHooks();
         if (!isset($hooks[$hookName])) {
             return self::CONTINUE;
@@ -137,7 +141,7 @@ class Hook
         foreach ($hooks[$hookName] as $priority => $hookList) {
             foreach ($hookList as $callback) {
                 $params = array_merge([$hookName], $args);
-                if (call_user_func_array($callback, array_merge([$hookName], $args)) === self::ABORT) {
+                if (call_user_func_array($callback, array_merge([$hook], $args)) === self::ABORT) {
                     return self::ABORT;
                 }
             }
@@ -195,6 +199,14 @@ class Hook
         static $calledHooks = [];
         return $calledHooks;
     }
+
+    protected static function possibleCallableToString(callable|string|null $callableOrString): string
+    {
+        if (is_string($callableOrString)) return $callableOrString;
+
+        $rf = new \ReflectionFunction($callableOrString);
+        return get_class($rf->getClosureThis()) . '::' . $rf->getName();
+    }
 }
 
 if (!PKP_STRICT_MODE) {

This might move us ahead in our quest for proper hook self-documentation and static analysis, and reduces the frequent repetition of class and function names when calling hooks that are often named identically to the current function.