[14:45:47] What is the "proper" way to get the RequestContext in a service? I see a lot of uses of RequestContext::getMain() but that seems wrong? [14:50:36] I guess maybe I'm confused on what RequestContext *is* [14:50:51] coming from Symfony, the request is created from the globals and passed into the application at the entry point: https://github.com/symfony/demo/blob/master/public/index.php#L24-L25 [14:58:10] and then you get the request out in your service via the "RequestStack" https://symfony.com/doc/master/service_container/request.html [14:58:36] but it doesn't look like the RequestContext is in the container? [14:58:36] or am I missing something? [15:20:56] davidwbarratt: duesen (who is not on IRC at the moment, it seems) would probably be the best person to discuss that with. [15:21:13] * anomie will be watching for the answer on that question too [15:23:14] duesen: Oh, I just mentioned you. davidwbarratt asked a question I think you'd have the best answers to. See https://wm-bot.wmflabs.org/logs/%23mediawiki-core/20190425.txt [16:27:29] davidwbarratt: in service wiring when constructing the service, if it needs RequestContext , using getMain() within servicewiring seems fine. It might become its own service one day. But more generally, I think one may want to avoid using RequestContext as service parameter in favour of taking individual objects it needs (User, WebRequest, Title, etc.) [16:28:42] RequestContext is a bit of a kitchen sink and hard to deal with, e.g. when you're in api.php, load.php, job queue or maintenance script, much of it doesn't make sense and becomes problematic. [16:37:24] I tend to get confused on that sort of thing, $wgUser versus RequestContext::getMain()->getUser() versus passing the same User to the constructor of something returned from MediaWikiServices doesn't seem like it's making much of a difference. [16:38:48] Most of the real $wgUser-removing I've seen seems to have been moving towards passing it as a parameter into methods rather than just shuffling the exact location of the global around. [17:18:47] davidwbarratt: the proper way is not using RequestContext :) [17:18:57] tgr hahahaha. :) [17:19:09] not necessarily feasible today, though [17:20:20] the analogue to Symfony's request object would be WebRequest; RequestContext is basically the entire global namespace (the request, the session user, the current title, the request language, the site configuration...) [17:20:40] ideally all those things do not belong together [17:21:29] Well, the session User, the "global" Title, and the language all are derived from the WebRequest. [17:22:06] except we don't have the services today which would take a WebRequest and return a title or user, which I guess would be the nice way to do this, at least at the controller level [17:22:11] (the UI Language usually indirectly via the User's preferences) [17:22:51] yeah, in the end the request and the site config are the only two real sources of input, everything else has to be derived from them in some way [17:23:20] and right now that derivation happens within RequestContext (sort of) [17:24:01] which is not a good pattern but there hasn't been a discussion yet on what to replace it with [17:24:33] OTOH, re-parsing the User and Title from the WebRequest each time might turn out to be a performance bottleneck. So we'd wind up in-process-caching them somewhere, whether WebRequest is the handle to that cache or IContextSource doesn't seem to make a huge amount of difference. More annoying is that we currently have some of both going on and they can get out of sync. [17:25:37] I imagine we'd have some kind of RequestUserService which takes a WebRequest and returns a User, and that would handle the caching [17:26:21] at least that's the current architecture direction: stateful objects should be simple, caching should happen in (otherwise) stateless services [17:29:37] $wgUser we are just in the process of killing (it's an extra level of bad because sometimes it's null and no one expects that; also for RequestContext::getUser you can do logging and deprecation warning and whatnot, and for a global you can't; also the whole StubObject mess) [17:29:54] RequestContext::getUser is probably not going away for a while [17:30:40] anomie: one tricky bit though, is how does api.php inject/decide what the title should be for its WebRequest? [17:31:00] and how does load.php inject/decide that it has no session; same for CLI/job queue. [17:31:42] there's potentially a case to be made for some services only having a factory in the service container, with the actual instance being created by an individual consumer based on its request context, thus alllowing that to be passed in from the entry point, separate from the service wiring which only depends on Config. [17:33:13] Krinkle: Yes, the way things are derived varies depending on the entry point. Sometimes it's not actually derived, e.g. api.php sets the global Title to "Special:Badtitle/dummy title for API calls set in api.php" and nothing is intended to actually use it. [17:33:52] anomie: yeah, that's a problem though if service wiring would require it by passing in RequestContext or getTitle() [17:34:06] I don't think we should have "forbidden services" in the service container. [19:45:14] Krinkle: https://gerrit.wikimedia.org/r/506544 [19:45:43] Reedy: :P https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/506541/ [19:45:55] ohhh [19:46:08] I didn't see that before your comment [19:46:09] ffs [19:46:25] Basically the same. Although I have some unfounded suspicion that an empty define() with a non-empty before() might do something weird, don't use Mocha enough to be sure. [19:46:55] Yeah phabricator strongly censors what it thinks you need to know.