I'm not sure I understand why the HookRunner wouldn't be in the service container itself? Is there a value to instantiating a new instance of the HookRunner for each service?

Hook runner is basically a convenience wrapper for HookContainer. It's core-only, extensions would have to define their own runner class for the hooks they define. HookContainer should be an implementation detail, it should not be exposed. The reason is mainly that, because HookRunner implements all the hook interfaces, it binds to a lot of different classes, and it would drag in these dependencies to anything that refers to it.

When we talked about it, Tim said that profiling suggests that there is no significant performance impact from instantiating several HookRunner instances.

I'm not a fan of injecting either HookContainer or HookRunner into service instances. Ideally, they would have specific hooks injected, one per hook. It would be the wiring code that would be dealing with HookContainer and HookRunner.

We could have an internal HookRunner service instance (using a "_" key in the wiring file), so the same instance could be re-used by several instantiators. But we shouldn't have a getHookRunner() method on MediaWikiServices.

In T254271#6212582, @daniel wrote:

We could have an internal HookRunner service instance (using a "_" key in the wiring file), so the same instance could be re-used by several instantiators. But we shouldn't have a getHookRunner() method on MediaWikiServices.

That seems like a good solution to me. I don't think it's wise to instantiate multiple instances just to keep it "private". Especially when nothing prevents an extension from instantiating the core HookRunner anyways.

While there may be no performance impact of instantiating multiple instances, I don't see a benefit to doing so regardless.

This question came up because of 601377. It's not clear if extensions should follow the same pattern as core or not. I don't see a reason why it couldn't be added to the service container (even if it's an "internal" service).

The RFC as originally written did have HookRunner as a service. @Anomie objected to that at T240307#5728354 and so I changed it to not be a service. This point was further reviewed by TechCom, and the RFC was approved in the present form. I thought it was approved by @dbarratt, since his comment at T240307#5771802 indicated that he had reviewed it -- that was the proper time for this kind of comment. There would have to be a very good reason to reopen the discussion after so much implementation work has been done.

This change proved to be beneficial in the call site migration patch, since only a single service (HookContainer) needs to be injected, not two services. HookContainer is needed for isRegistered(), so if you want both isRegistered() and HookRunner, you need both objects.

In T254271#6212593, @dbarratt wrote:

In T254271#6212582, @daniel wrote:

We could have an internal HookRunner service instance (using a "_" key in the wiring file), so the same instance could be re-used by several instantiators. But we shouldn't have a getHookRunner() method on MediaWikiServices.

That seems like a good solution to me. I don't think it's wise to instantiate multiple instances just to keep it "private". Especially when nothing prevents an extension from instantiating the core HookRunner anyways.

A solution to what? No problem has been described.

If it's about convenience, Daniel's requirement that there be no getHookRunner() prevents any convenience benefit.

This question came up because of 601377. It's not clear if extensions should follow the same pattern as core or not. I don't see a reason why it couldn't be added to the service container (even if it's an "internal" service).

There are several methods for obtaining a HookRunner instance in core, aimed at convenient access. You can follow one of them in CheckUser, or do your own thing.

I just realized that the HookRunner in the patch is CheckUser's own hook runner. I'd argue that extensions can handle their own hook runners as they please. CheckUser could add a CheckUserHookRunner service to the service container, or instantiate HookRunners on the fly, as seems convenient. CheckUser could also make a CheckUserServices class that wraps MediaWikiServices (or just has its own wiring) to provide a getHookRunner() method. The things I said above apply to core's HookRunner, and to core's MediaWikiServices and service wiring.

In T254271#6231199, @tstarling wrote:

There would have to be a very good reason to reopen the discussion after so much implementation work has been done.

It seems like it would require a minimal amount of work to add the HookRunner to the service container (none of the existing implementations would need to be changed).

This change proved to be beneficial in the call site migration patch, since only a single service (HookContainer) needs to be injected, not two services. HookContainer is needed for isRegistered(), so if you want both isRegistered() and HookRunner, you need both objects.

You would still need both objects regardless if they are injected or not. It's just moved the injection to instantiation, which doesn't seem to provide any benefit (from what I can tell).

A solution to what? No problem has been described.

If it's about convenience, Daniel's requirement that there be no getHookRunner() prevents any convenience benefit.

I don't think that's the case. Services can obviously be injected without a named method existing on the container. I think the problem is having to know about the implementation details of HookRunner. The need to be aware of, and inject, HookRunner's own dependencies, seems like an unnecessary step with no benefit.

In T254271#6231772, @daniel wrote:

I just realized that the HookRunner in the patch is CheckUser's own hook runner. I'd argue that extensions can handle their own hook runners as they please. CheckUser could add a CheckUserHookRunner service to the service container, or instantiate HookRunners on the fly, as seems convenient. CheckUser could also make a CheckUserServices class that wraps MediaWikiServices (or just has its own wiring) to provide a getHookRunner() method. The things I said above apply to core's HookRunner, and to core's MediaWikiServices and service wiring.

That makes perfect sense to me. Although, @Anomie's comment in T240307#5728354 seems to imply the exact opposite. :/

In T254271#6231852, @dbarratt wrote:

That makes perfect sense to me. Although, @Anomie's comment in T240307#5728354 seems to imply the exact opposite. :/

I don't share the concern of cluttering the service container. There would have to be a hundreds of extensions defining hook runners for that to become a problem.

In any case, the HookRunner should indeed be an internal utility, not used by other extensions. It could be registered in the service container or not.

In T254271#6231845, @dbarratt wrote:

It seems like it would require a minimal amount of work to add the HookRunner to the service container (none of the existing implementations would need to be changed).

That's true, the problem isn't the amount of work. The problem isn't even having a HookRunner in the service container, being used only inside the wiring. The thing I want to avoid is having a getHookRunner() method. This would make HookRunner supported infrastructure, rather than an internal utility.

In T254271#6231885, @daniel wrote:

That's true, the problem isn't the amount of work. The problem isn't even having a HookRunner in the service container, being used only inside the wiring. The thing I want to avoid is having a getHookRunner() method. This would make HookRunner supported infrastructure, rather than an internal utility.

I think that's totally acceptable. In fact I wonder if there are more services that should be treated this way?

This reminds me of Symfony's concept of Public vs Private services. Effectively, "private" services can only be injected. They cannot be accessed directly from the container. They even insist that private services are the norm, rather than the exception. Perhaps it would be wise to make a pattern like this in MediaWiki?

In T254271#6231943, @dbarratt wrote:

This reminds me of Symfony's concept of Public vs Private services. Effectively, "private" services can only be injected. They cannot be accessed directly from the container. They even insist that private services are the norm, rather than the exception. Perhaps it would be wise to make a pattern like this in MediaWiki?

Informally, this exists. Look for service names that start with "_" in ServiceWiring.php.

That being said: I wouldn't want to see HookRunner in the type hints for a service's constructor signature. The service should ask specifically for the hook interfaces it needs (or for a HookContainer, if it needs to do more than just trigger the hook). The wiring code can then satisfy that need as it pleases, with a new HookRunner, or a shared one.

In T254271#6231956, @daniel wrote:

That being said: I wouldn't want to see HookRunner in the type hints for a service's constructor signature. The service should ask specifically for the hook interfaces it needs (or for a HookContainer, if it needs to do more than just trigger the hook). The wiring code can then satisfy that need as it pleases, with a new HookRunner, or a shared one.

That makes perfect sense to me.