Problem
As a follow up to T240307: Hook container with strong types and DI
While working on T251465, we realized that there was a need for implementers of the hook to have access to the current request context RequestContext.
Questions
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | tstarling | T252798 How should hook implementations access the current request context? | |||
| Resolved | tstarling | T240307 Hook container with strong types and DI | |||
| Resolved | apaskulin | T246855 Hook interface doc comment review | |||
| Open | None | T250859 Rename BeforeParserrenderImageGalleryHook to BeforeParserRenderImageGalleryHook | |||
| Resolved | tstarling | T252686 extension.json did not pass validation: The property HookHandlers is not defined and the definition does not allow additional properties | |||
| Open | None | T253768 No easy way to suppress hard-deprecation warnings for hooks | |||
| Resolved | BUG REPORT | Reedy | T254053 MediaWikiTestCaseTrait::createHookContainer failing mediawiki-quibble-composer-mysql-php72-docker | ||
| Duplicate | None | T257196 Allow tests to reset/clear non-legacy hook handlers | |||
| Resolved | • nnikkhoui | T258665 Document canonical hook name in hook interfaces |
Historically, it seems that hooks were updated to pass the request context to the handlers when needed, for instance in rMW1051f68e7d438d20df44fd9386a0a451e77fbb8e. Given that request context is not suitable for injection as per T218555, this strikes me as probably the most suitable approach.
Conceptually, I agree. Sadly, RequestContext in it's current state is overburdened and drags in things it should not, such as the Skin object, or the Config object, or a WikiPage. The respective getters should probably be deprecated. But we don't really have an alternative right now.
… or a Title, or a User, or a WebRequest. None of these are universally safe or make sense to be able to use from (all) hooks. See also T218555.
Any specific parts of the context, if the extension's response is allowed to vary by it, should be passed to the hook explicitly. And for hooks that fire exclusively during UI-facing web requests (e.g. Skin/SpecialPage/OutputPage etc.) that could even be as broudly as passing the entire object or context in - which a lot of hooks already do.
For anything else, I'm not sure that formalising an fundamentally unsafe concept is beneficial, beyond what we have which is that you call RequestContext::getMain() and thus know that it is sometimes wrong or unsafe.
As an example, there is no guruantee that the component's specific concept of titles or users will match the global one. It is not safe to assume that a hook fired from revision insertion will be about the "current" title. It might be a batch operation from the JobQueue or CLI, or an undelete or xml import from a special page or API request, etc.
Any specific parts of the context, if the extension's response is allowed to vary by it, should be passed to the hook explicitly.
I think it would make sense to define a narrow interfaces that can be implemented by or wrapped around ContextSource. This would expose only narrow, serializable object, such as UserIdentity. This should probably be discussed in the context of T231930. I note that a filter hook may want access to an "authority" object which may not be serializable, while an action hook (i.e. a listener) can only depend on serializable data.
As everyone else said, the relevant information should be sent in parameters, not implicitly injected like a service. Use of RequestContext::getMain() in a hook handler is problematic since it makes it difficult to later change the UI.