Page MenuHomePhabricator

Provide access to WebRequest and associated information via a service object
Open, Needs TriagePublic

Description

Currently, application logic accesses information about the current request (such as the identity of the current user, the requested page, etc) primarily via a RequestContextSource singleton obtained from RequestContext::getMain(). RequestContextSource in turn binds to "heavy" classes like Title and User, and RequestContext relies heavily on global state. It also doubles as an i18n facility, implementing MessageLocalizer.

This should be replaced by a RequestInfo service object, that can be injected (or obtained from the global service locator if need be). RequestInfo would be structurally similar to RequestContext, but would bind to lightweight interfaces instead of heavy classes, and would not rely on global state.

Note that ideally, any information derived from the request should be passed to application logic as a parameter, and should not be known to service instances. However, changing MediaWiki's architecture in that regard seems difficult, and PHP's per-request execution model makes it acceptable to treat the user request as a global thing, making it available via a service instance. The same could perhaps be said about the response, but treating the response as global seems more problematic, since it is mutable.

RequestInfo should implement the following methods (similar to the ones found in RequestContext):

  • getRequest(): WebRequest. Note however that WebRequest binds to Session, which binds to User. Session::getUser should in time be replaced with Session::getUserIdentity to fix this.
  • getTarget(): LinkTarget. Replaces getTitle().
  • getUserIdentity(): the identity of the logged in user, if any
  • getRequestedLanguage(): get the code of the requested output language. Based on the user's preferences, but may be overwritten per request. Avoid binding against the heavy Language class, though.
  • getRequestedSkin(): get the name of the requested skin. Based on the user's preferences, but may be overwritten per request. Avoid binding against the heavy Skin class, though.
  • getDiagnostics(): returns a human readable array of diagnostic information about the request, similar to RequestContent::exportSession(). The contents should be treated as serializable, opaque, and for human consumption.

RequestInfo should not implement the following methods found in RequestContext:

  • getOutput() - while input can be treated as a global thing, output generally should not.
  • getStats(), getTiming() - use separate services or aggregate in some kind of output object
  • getWikiPage() and canUseWikiPage - WikiPage (or, in the future, PageRecord, see T195069) should be obtained from a storage service.
  • getConfig() - this has nothing at all to do with the request
  • msg() - localization should be done elsewhere.

RequestInfo could implement the following additional methods in the future:

  • getSession(): Same as getRequest()->getSession(). WebRequest::getSession could perhaps be deprecated in the future, to turn WebRequest into a value object.
  • getPageIdentity(): PageIdentity, once PageIdentity exists (T208776). Very similar to getTarget(), perhaps not needed.
  • isLoggedIn(): whether the request comes from a logged in user.
  • isWebRequest(): whether this is actually a web request, as opposed to a CLI script or async job execution.

Event Timeline

daniel created this task.Mar 18 2019, 11:10 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 18 2019, 11:10 AM

So it's very easy to write the class, but I can't figure out where to use it. Most RequestContext users seem to use it for config and messages and such, inter alia, so there aren't many places I can see where it makes a lot of sense to just substitute RequestInfo. It seems like you'd want to start from classes like LogFormatter and Message, and the first step there should probably be to make them be constructed by services.

The main motivation right now is to have a clean way to inject access to the WebRequest into PermissionManager and friends.

There is no use of WebRequest in PermissionManager right now. The only use of RequestContext is to pass it straight along to Action, which exposes it to subclasses that could conceivably be in extensions. Could you provide more details on what you want to use it for?

daniel added a comment.EditedApr 15 2019, 6:12 PM

@Simetrical sorry, yes. See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/502484/16/includes/Permissions/PermissionManager.php#1174. The proposed BlockManager also uses WebRequest, see https://gerrit.wikimedia.org/r/c/mediawiki/core/+/502820/5/includes/block/BlockManager.php#14, but it's passed in as a parameter. Which raises teh question where the caller gets it from, if not from global state.

There are other instances of code in need of servisification that needs a WebRequest, e.g. static function buildRollbackLink in Linker, and static function sendHeaders in ContentSecurityPolicy. Both of them (can) get the RequestContext as a parameter. That is theoretically the cleaner pattern, but would require us to pass the RequestContext (or RequestInfo or something) around pretty much everywhere. Given PHP's per-request execution model, it seems saner to just inject it into the service instances.

Oh, I just found static function getRequestId() in WebRequest, which is also called quite a lot...

Why not inject the WebRequest directly instead of putting it inside a wrapper that includes stuff about the user, language, and skin? I.e., what's wrong with just MediaWikiServices::getWebRequest()?

daniel added a comment.EditedApr 16 2019, 11:37 AM

Why not inject the WebRequest directly instead of putting it inside a wrapper that includes stuff about the user, language, and skin? I.e., what's wrong with just MediaWikiServices::getWebRequest()?

Two reasons: 1) it doesn't quite contain everything we want and 2) the wiring code needs to get it from somewhere - so it would have to be treated as a pseudo-service, with a getWebRequest method in MediaWikiServices.

Both issues can be solved, but those are the reasons I thought it would be better to wrap it in a service. But I'm open to injecting it directly. I just want a clear way to do this without resorting to global state.

By the way, public static function overrideRequestId( $id ) in WebRequest seems problematic... or at least, doesn't fit with the idea that services should not have setters or mutable state.

  1. Like what?
  1. Is there a problem with having getWebRequest in MediaWikiServices?
  1. Like what?
  1. The identity of the current User, and the things it provides access to. Since we want to avoid (publicly) binding to the User class, we'd need another way to do token checks and rate limits. Other things, like user settings, should be accessed via a separate service, given the user's identity. The language and skin preferences are an exception to that though, since they can be overwritten per request. The skin isn't so relevant, but having easy access to the effective user language via something like RequestInfo would be good.
  2. The identity of the requested page (as a LinkTarget, probably, to avoid binding to Title).
  1. Is there a problem with having getWebRequest in MediaWikiServices?

Not really, but if we need RequestInfo anyway to provide access to the title and user, there is no point in exposing WebRequest separately.

If the pieces of information are completely unrelated, why not expose them separately, like MediaWikiServices::getInstance()->getUserIdentity() and so on? What do we gain by bundling them in one unit? There are certainly tons of places that use $wgUser or RequestContext::getMain()->getUser() without having any need for a WebRequest, for instance.