Change 329691 had a related patch set uploaded (by Daniel Kinzler):
Introduce HookRunner

https://gerrit.wikimedia.org/r/329691

I wonder why this solution is chosen rather than injecting the HookManager service you describe in T154677: Turn static Hooks class into HookManager service and calling a run() method on that.

Because it's good practice to only inject as little as possible. The HookManager is a global registry. You could call any hook on it. That's the "kitchen sink" anti-pattern: inject everything, let the implementation pick and choose. We should instead follow the "gaia principle": ask exactly for what you need.

Injecting the HookManager directly would already be a step in the right direction, but it would not allow us to move on to using a listener interface. With HookRunner, the step to a listener interface is streight forward.

HookRunner as its own thing still seems unnecessary to me. I'm wary that the "step to a listener interface" would become a permanent layer of abstraction.

As a SimpleListenerBase class where run() is a protected method intended to be called from public function onHook( ... ) { $this->run( [ ... ] ); }, sure, that might work.

I'm also wary that your whole design for hooks-as-a-service, as far as I can infer it from the current patch, isn't going to handle the existing use cases. In particular, it doesn't at all handle hooking after the HookRunner is created, or unhooking at all.

The HookRunner is essentially the same as your SimpleListenerBase base class. I just prefer composition over inheritance for code reuse, because it is more flexible, and allows more fine grained unit tests. Using inheritance to gain access to some protected utility method is rightly considered a code smell, I would like to avoid that. Also, with SimpleListenerBase, the listener can only wrap the handlers for a single hook (or for all hooks), not for two specific hooks. With HookRunner composites this is straight forward.

it doesn't at all handle hooking after the HookRunner is created, or unhooking at all.

If HookManager is a service, then these things are no longer possible via wgHooks. But they are possible via the HookManager service.

But anyway - yes, this may prove problematic, and that's why I am proposing HookManager independently of the injection mechanism. I think turning Hooks into a service is going to be somewhat tricky, and may take a while. Introducing HookRunners and/or listener interfaces is straight forward, can be done interatively, and is fully backwards compatible.

Perhaps I should have not mentioned HookManager at all, it's kind of misleading wrt the issue of dependency injection.

...that is to say: for now, I'd rather leave HookManager than HookRunner. If we don't have HookRunner as a standalone composite, we'll need it as a base class.

Introducing HookManager doesn't have that much benefit, and migrating everything to using HookManager instead of Hooks and wgHooks is going to be a pain. But introducing HookRunner and/or listeners has an immediate benefit for whatever service class is starting to use it.

On the other hand, composition means you have a pile of different objects that you have to trace through each layer of abstraction to figure out what's going on. At least HookRunner isn't that complicated, I think we can agree to disagree here and wait for others to weigh in.

If HookManager is a service, then these things are no longer possible via wgHooks. But they are possible via the HookManager service.

I haven't seen a patch actually implementing a HookManager service yet, or even a proposed design, so I don't have any idea of how it might work or how it might interact with these HookRunners.

Introducing HookRunners and/or listener interfaces is straight forward, can be done interatively, and is fully backwards compatible.

Until you turn some existing hook that needs late registration or unregistration into a HookRunner and things mysteriously break because that can only be handled through HookManager and HookManager doesn't exist yet.

From discussion with Brad at the DevSummit:

• there is a need to dynamically register/unregister hooks. It's not nice, but for some use cases, we don't have a great alternative.
• so HookRunner hsould have register( $handler, $name = null ) and unregister( $name ) methods.
  • Maybe a magic scoped callback could be used for automatic unregistering.
• This means HookRunner instances should be singletons (getRunner should always return the same instance for a given hook name)
• The fact that HookRunner works on a copy of the list of hook handlers causes a chicken-and-egg issue in the following case:
  • service Foo calls hook Xyzzy, and thus gets a HookRunner for Xyzzy in the constructor.
  • bootstrap code registers a closure for handling Xyzzy
  • that callback needs to know the Foo service
  • getting the Foo service, so we can pass it to the callback before registering it as a handler for Xyzzy, causes the Hookrunner for Xyzzy to be instantiated and injected into the Foo service
  • registering the closure in wgHooks (or in Hooks::register) has no effect, because the HookRunner was already instantiated
• possible solutions:
  • don't use HookRunner for hooks that can have this problem. Warn in documentation
  • Make the Foo service a parameter to the hook call, so the closure doesn't need to know it at bootstrap time
  • Provoide a promise to the closure, instead of the actual Foo service
  • make the HookRunner not use a copy, but a live reference
  • make wgHooks a magic ArrayAccess object that actually registered the hander in the HookRunner object. (This is the most complete solution, but smells of too much magic)

See also related notes on T154677

• there is a need to dynamically register/unregister hooks. It's not nice, but for some use cases, we don't have a great alternative.

I disagree with that and think most/all cases of needing this are to work around other bad global state code. I don't think we should encourage dynamic hook registration (outside of tests).

Make the Foo service a parameter to the hook call, so the closure doesn't need to know it at bootstrap time

Note though that that doesn't solve situations where the callback needs Bar service and that service needs Foo, while Foo doesn't care anything about Bar.

I disagree with that and think most/all cases of needing this are to work around other bad global state code.

If you're volunteering to fix all instances of such things in a timely manner before this proposal can move forward, feel free. As things stand, the requirement exists.

In T154674#2938504, @Anomie wrote:

I disagree with that and think most/all cases of needing this are to work around other bad global state code.

If you're volunteering to fix all instances of such things in a timely manner before this proposal can move forward, feel free. As things stand, the requirement exists.

I think you're misunderstanding my comment. I said "I don't think we should encourage it". That means not necessarily providing a nice API for it and requiring people to hack around it, which would be equivalent to the status quo I think.

There's not really any hacking around the hook system to do it currently, since it's just setting and unsetting things in $wgHooks. The current patch in Gerrit makes it completely impossible to do.

I'm not convinced this is a good direction. We depend very heavily on the ability to grep for hook calls. If we had to track injection chains instead to figure out where a given hook is called, that would make maintenance rather difficult. Plus it does not solve the biggest problems we have with hooks (lack of type safety, awkwardness of documentation, no good way to handle changes in signature, clear understanding of when global state is being used).

How about something like this instead: we group hooks by function, each group gets a dedicated runner, so e.g. there is something like

UploadHookRunner extends AbstractHookRunner {
    public function onUploadVerification( $saveName, $tempName, &$error ) { $this->run( 'UploadVerification', [ $saveName, $tempName, &$error ] ); }
    public function onUploadVerifyFile( $upload, $mime, &$error ) { $this->run( 'UploadVerifyFile', [ $upload, $mime, &$error ] ); }
    public function onUploadComplete( &$image ) { $this->run( 'UploadComplete', [ &$image ] ); }
}

which you can fetch from MediaWikiServices.

This is type-safe, can be properly documented in a way that is understandable by IDEs, would go a long way towards making our hook system easier to navigate, remains easy to grep. Breaking changes can be handled by simply introducing a new UploadHookRunnerV2 class with a different signature so that clients can update at their leisure and it's easy to mine information on whether they did. And global state could be limited to the hooks where we actually rely on hacks (the rest simply would not have setters for new handlers) which could be gradually phased out.

(I guess this is what @Anomie was referring to in T154674#2920309.)

I came with a simple solution that allows us to annotate Hook callbacks T171515.

The old way is to specify Class::hookHandler, I came with the idea of adding a simple annotation. In Class::hookHandler first part is a class name, instead of classname we can pass @ServiceNamem then the hook definition would look like: @Service::onHookHandler. Hooks::run() will check the first character, when @ (the service annotation) is passed Hooks::run() will use MediaWikiServices to retrieve the service instance and create valid callable.

@Anomie @daniel @Tgr any comments on the above (T171515)? IMHO it's the easiest and pretty clean solution. Passing service name is almost the same as passing the class name. Only callback definition changes, everything else stays the same.

Regarding current Hooks system, I'm not a fan of Symfony framework as it has lots of bloat. But I have to admit I really like their dependency injection infrastructure, especially service tagging. I find it very useful, as it can be used for many different things, like - hooks/factories/loggers and much much more. Having a possibility to tag a service, and retrieving all services with the same tag at once is neat. Couple examples:

• we could register current hooks as services, tagged with hook.* where * stands for hook name, then HookRunner would retrieve all services for given tag (ex: MediaWikiServices::getAllForTag('hook.GetPreferences')). Question is, do we want to do one class per hook, or a HooksContainer with set of hook listeners and HookRunner would call $service->onGetPreferences()
• we could create nice factories, each factory instead of having hardcoded switch(){case} would retrieve all services with a specific tag (that only this one factory uses)
• registering additional listeners (logging?) would be pretty easy as extension could tag a service, and then main logger class would retrieve all loggers for given tag, and do log() on each.

HookManager sounds really good, but it's just another wrapper to store all hooks data with global access. We already have MediaWikiServices, why not use it for store everything.

RFC backlog for now, I need to update the proposal to address the comments I got here.

Change 329691 abandoned by Daniel Kinzler:
Introduce HookRunner

Reason:
attic

https://gerrit.wikimedia.org/r/329691

@daniel Is this still applicable with RFC T240307 approved? Maybe there is still part of this we can do as follow-up, or is it obsolete?