Config on the RequestContext may not be the same as the main config
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	dbarratt
	Dec 6 2019, 3:17 PM

Description

Problem
In 92004 a setConfig() method was added to RequestContext.

In 137211 a getConfig() method was added to SpecialPage. Though, the assumption (from the comment) is that this method will always return the "main" config.

However, because of RequestContext::setConfig() that is not always the case. This could create a discrepancy between the config that is retrieved from the RequestContext and the config from the MainConfig service.

Solution
It does not appear that anything in core (outside of tests) is using RequestContext::setConfig(). It may be simplest to deprecate this method. Alternatively, SpecialPage::getConfig() could be altered use the MainConfig service rather than the config on the RequestContext.

Event Timeline

dbarratt created this task.Dec 6 2019, 3:17 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 6 2019, 3:17 PM

dbarratt added a project: Anti-Harassment.Dec 6 2019, 3:23 PM

Restricted Application added a subscriber: MGChecker. · View Herald TranscriptDec 6 2019, 3:23 PM

dbarratt updated the task description. (Show Details)Dec 6 2019, 3:30 PM

Going one step further: config being both a request property and a service seems weird. I think we should make up our mind about which one it is. I'd suggest that the config should be a service, not a request property, and so we should not just deprecate RequestContext::setConfig(), but also (soft-)deprecate RequestContext::getConfig() and move away from it in favor of the MainConfig service. There is a little bit of precedent for this, we deprecated ResourceLoaderContext::getConfig() this year.

This would be a messy migration though, because RequestContext::getConfig() is used all over the place.

Anomie moved this task from Inbox to Triage Meeting Inbox on the Platform Engineering board.Dec 9 2019, 2:36 PM

Is this an area where documentation can be expanded or instantiated?

@Catrope That totally makes sense to me, it would be great if we picked a direction either way.

In T240000#5865613, @WDoranWMF wrote:

Is this an area where documentation can be expanded or instantiated?

I don't think so. I think there is a (theoretical?) problem that should be resolved. At a minimum, a direction should be decided upon.

Let's deprecate RequestContext::getConfig(). The config has nothing to do with the request. Per-request information should be passed as a method param, config should be injected into the constructor.

@dbarratt does this answer your question? Feel free to close or keep open.

This was discussed in last week's TechCom meeting. TLDR:

RequestContext predates MediaWikiServicing.
For Config, MediaWikiServicing is indeed the better place for it.
For code that is itself, or is closely coupled with, user/web-facing code this can continue to be passed down and/or made available via the RequestContext interface that subclasses of Action and SpecialPages implement, which can't be a service, and thus for those $this->getConfig remains the best recommended practice.
For anything else, including the entry point files and internals of RequestContext itself, the service container should be preferred.

So outside Action/SpecialPage classes, where one can't safely use a RequestContext, the config should be obtained from the service container. And long-term direction is that entry points instantiate RequestContext explicitly instead of lazily like it is now, and thus they would inject Config from the service container there.

That logic would likely live in index.php or MediaWiki.php, which is quite well abstracted away from the rest, but within itself indeed quite messy still and not yet in line with that long-term direction.

For now I'm closing this as the question is answered, but if you want to re-use this task for the fulfilment of that direciton feel free to re-open, or file a separate task.

Config on the RequestContext may not be the same as the main configClosed, ResolvedPublicActions

Description

Event Timeline

Config on the RequestContext may not be the same as the main config
Closed, ResolvedPublic
Actions