Replace hooks by events

[jonasraoni]

2022-08-24: This proposal was declined due to the lack of priority management in Laravel events. See this thread.

As we're adopting Laravel, I think it makes sense to replace the hooks by events.

• The events run synchronously (unless we specify that a handler should be executed in a queue).
• It's possible to cancel the propagation of the event, which is a good thing to have for backwards compatibility.
• The interface is simple to use (Event::dispatch()/listen()).

Pros:

• As the events are based on classes, instead of strings, invalid/removed events will be flagged earlier.
• The available attributes will be visible and typed (at this moment we just receive an $args from the hook).
• Side-effects can be made clearer by using methods (e.g. cancel()) or attributes, instead of manipulating variables passed by reference.

Cons:

• Backwards compatibility will be needed.

[NateWr]

Thanks @jonasraoni. There's an issue filed to review this for 3.4, so that we can try to get things organized as soon as possible. #7831

I think there's a good case for keeping hooks and events, but using them for separate cases. From the issue:

In my view, hooks should be used whenever a plugin may wish to intervene before an action is taken. Examples: when an object is validated or about to be added, edited or deleted; when getting a list of valid decisions; "lifecycle" hooks, like when a handler is loaded, before a template is rendered, etc.

Events should be used when an action has already been taken, and for any asynchronous actions. Examples: after an object is added or edited or deleted; after a submission or issue is published; after a job is run.

[jonasraoni]

Oh, next time I'll search before writing 😅

Considering that events can fully replace the hooks (they are synchronous by default + the propagation can be stopped), while bringing the PROs that I've wrote, what's the reasoning for keeping both?

I think the concept you've mentioned (hooks = before the action, events = after) is enough to cover with a good naming standard BeforePublishEvent/AfterPublishEvent.

[nils-stefan-weiher]

Another Pro: The event classes are easily identified by an IDE and you can create updated documentation automatically. The documentation for hooks is basically non existing and the best approach to find hooks in the codebase is a string search for HookRegistry::call and sometimes this is not enough because of Hooks placed in parent classes.

[NateWr]

I agree that type hinting is a huge plus, and events are better than hooks in that sense. I have a couple of doubts that I would want addressed before jumping to events.

1. There are hundreds of hooks in the system (285 by my quick search). How would we organize 285 event classes so that they are discoverable? While a hook lacks good documentation and code completion, we have some naming conventions which mean they are easier to find once you understand the most common ones. And it keeps more significant events (like PublishedEvent) easier to find than it would be in a directory with hundreds of classes. Maybe we could call some events hooks? So we'd have classes like PKP\observers\hooks\ExampleHook?

2. We should compare the performance of hooks and events. Because hooks are used throughout our extensible data model (look at examples in db/DAO.inc.php), they are called hundreds or thousands of times in a single request. This is not really what Laravel's events model was designed for, so we'd need to see how it handles these cases.

3. At the moment, core event listeners are cached by the system. Plugins can register event listeners at run time. How would this perform when plugins are potentially registering 100+ events at run time?

[nils-stefan-weiher]

Hi @NateWr ,

thanks for your detailed writing.
Thinking over it once more I agree, that It could be too much clutter in the filesystem. Especially when I think about the Smarty template hooks, which actually would be a very bad fit for an event class.

Maybe for content display we should keep the HookRegistry and for processing related hooks, e.g. Form processing and Service methods we could think about a naming convention for events like @jonasraoni proposed "BeforeXXX" "AfterXXX" ?

[jonasraoni]

I think performance isn't going to be an issue, the codes below have essentially the same performance (which will be our case after switching from hooks to events), but I'll check one thing before giving my final opinion :)

for(var i = 0; i < 1000; ) {
  processA();
  processB();
}

for(var i = 0; i < 1000; ) {
  processA();
}
for(var i = 0; i < 1000; ) {
  processB();
}

[asmecher]

I don't think that's a simplification of the performance issue @NateWr is suggesting; it's the matter of loading event classes, registering listeners, and processing callbacks.

[NateWr]

I've just discovered that Laravel events do not support priority. In other words, when an event has two listeners, there's no way to declare that one should run before the other. Event priority was removed in 5.4 and it's not coming back.

In my view, this is a requirement for many hooks. We often want to a hook to be able to say "run me before most other callbacks" or "run me after other callbacks". An example of this would be the Decision::types hook which allows the list of available hooks in an app to be modified. I may have two plugins enabled that modify the list. One plugin adds decisions and the other plugin removes decisions. The second plugin should be able to pass HOOK_SEQUENCE_LAST so that it can remove decisions added by other plugins.

[jonasraoni]

@NateWr As that's a requirement, I can't think of anything better than a priority queue (our implementation), to handle the execution order of unknown code, so this discussion can be closed (extending the Illuminate\Events\Dispatcher won't bring a great developer experience).

About closing, perhaps we should create extra categories (e.g. Rejected/Implemented), to work as a close button 🤔

[NateWr]

Good idea. However, the categories can't be restricted. Anyone can create a discussion in any category. So it doesn't seem quite right for this use. And I don't want to add labels since they appear for issues too.

I've settled on locking the discussion for now.

[NateWr]

(Never mind, I figured out how to make a closed category. So now we have "Declined Proposals".)