Page MenuHomePhabricator

How should hook implementations access the current request context?
Closed, ResolvedPublic

Description

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

  1. Should a hook pass the request context to the implementations?
  2. If not, how should the implementation retrieve the current request context? RequestContext::getMain() ?

Event Timeline

dbarratt created this task.May 14 2020, 4:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 14 2020, 4:34 PM
dbarratt renamed this task from How should hook implementations access request context? to How should hook implementations access the current request context?.May 14 2020, 4:34 PM
dbarratt added a project: Anti-Harassment.
dbarratt updated the task description. (Show Details)
TK-999 added a subscriber: TK-999.EditedMay 14 2020, 6:03 PM

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.

daniel added a subscriber: daniel.May 14 2020, 7:59 PM

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.

BPirkle triaged this task as Medium priority.May 19 2020, 8:37 PM
BPirkle added subscribers: tstarling, BPirkle.

Is there anything for CPT to do here? @tstarling

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.

… 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.

daniel moved this task from Inbox to Watching on the TechCom board.May 26 2020, 11:40 AM

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.

tstarling closed this task as Resolved.May 28 2020, 10:27 PM
tstarling claimed this task.

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.