Parser code for special page transclusion replaces main context
Open, Needs TriagePublicBUG REPORT
Actions

Assigned To

None

Authored By

	Tgr
	Sep 10 2021, 3:18 AM

Description

In Parser::braceSubstitution(), the block that handles transcluded special pages does this:

// Create a new context to execute the special page
$context = new RequestContext;
$context->setTitle( $title );
$context->setRequest( new FauxRequest( $pageArgs ) );
if ( $specialPage && $specialPage->maxIncludeCacheTime() === 0 ) {
	$context->setUser( $this->userFactory->newFromUserIdentity( $this->getUserIdentity() ) );
} else {
	// If this page is cached, then we better not be per user.
	$context->setUser( User::newFromName( '127.0.0.1', false ) );
}
$context->setLanguage( $this->mOptions->getUserLangObj() );
$ret = $this->specialPageFactory->capturePath( $title, $context, $this->getLinkRenderer() );

and then SpecialPage::capturePath() does this:

$main = RequestContext::getMain();
$ctx = [
	'title' => $main->getTitle(),
	'output' => $main->getOutput(),
	'request' => $main->getRequest(),
	'user' => $main->getUser(),
	'language' => $main->getLanguage(),
];

// Override
$main->setTitle( $title );
$main->setOutput( $context->getOutput() );
$main->setRequest( $context->getRequest() );
$main->setUser( $context->getUser() );
$main->setLanguage( $context->getLanguage() );

// The useful part
$ret = $this->executePath( $page, $context, true, $linkRenderer );

// Restore old globals and context
$main->setTitle( $ctx['title'] );
$main->setOutput( $ctx['output'] );
$main->setRequest( $ctx['request'] );
$main->setUser( $ctx['user'] );
$main->setLanguage( $ctx['language'] );

This was a hacky but reasonably safe thing to do in the past. But in a service container based world, where services can are created on the fly and stored for future invocations, and dependencies get passed in, all these things might get cached in various places. The one I ran into was MinervaHooks::onSpecialPageBeforeExecute() fetching the Vector.FeatureManager service, which is initialized using the request context; it ends up storing the FauxRequest created by the parser and so you get a weird failure of the useskinversion URL parameter on some pages.

That specific bug is very fringe and is not worth much attention, but the trick employed by the parser could be the source of other bugs and it might be worth thinking about how to move away from this pattern.

Related Objects

Mentioned In: T323184: SpecialPage transclusion triggers deprecated "Unexpected clearActionName after getActionName"
T314008: Promote dirty RequestContext::clearActionName to runtime warning
T288113: Legacy Vector preferences not respected on pages that include special pages (eg. Special:PrefixIndex)
Mentioned Here: T249668: 'LinkRenderer' service violates to not inspect request/session
T297203: Expand or create a new structure test to test for use of ContextSource in SpecialPage constructor
T288113: Legacy Vector preferences not respected on pages that include special pages (eg. Special:PrefixIndex)
T290539: Page randomly switches to New Vector after edit preview

Event Timeline

Tgr created this task.Sep 10 2021, 3:18 AM

Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald TranscriptSep 10 2021, 3:18 AM

@daniel I couldn't find the right tags but maybe this is interesting to you.

We've already seen this happen: T288113: Legacy Vector preferences not respected on pages that include special pages (eg. Special:PrefixIndex) (and T290539).

fetching the Vector.FeatureManager service, which is initialized using the request context;

This is why service wiring should not depend on the request or user settings.

In T290706#7343786, @Tgr wrote:

@daniel I couldn't find the right tags but maybe this is interesting to you.

Yes, thanks!

@tstarling This seems relevant in the context of the discussion we had the other day about service wiring depending on the request.

and then SpecialPage::capturePath() does this:

In an ideal world, SpecialPageFactory::capturePath() should not need to mess with global state. It should just construct a fake RequestContext and call setContext() on the SpecialPage before executing it. That should(tm) work. And the Real Fix(c) is to make it work...

Tgr mentioned this in T288113: Legacy Vector preferences not respected on pages that include special pages (eg. Special:PrefixIndex).Sep 10 2021, 7:59 PM

In T290706#7344048, @daniel wrote:

This is why service wiring should not depend on the request or user settings.

Ideally, but not having any dependency on user preferences (often used as feature flags to disable big chunks of code to reduce various risks associated with new features) or a message localizer seems very far from our current reality.

I can see three way to handle this:

Services do not depend the context; instead the context gets passed around basically everywhere as a function parameter.
Ability to set a faux service container when you use a faux context.
Providing the request context as a service, so other services can depend on that and always see the current value of the context.

The first is not realistic, at least in the short term; and even in the long term, I think it could prove pretty inconvenient. The third doesn't help when you'd want to initialize your service with something more granular, like the value of a user preference, or a query parameter. Having some ScopedCallback-style pattern which sets you up with a new context and a new service container seems the cleanest, but would probably have severe performance implications, and some services might expect to be called back in some teardown phase (e.g. a deferred queue) so not doing that might result to loss of consistency.

So none of those seem like great options...

ServiceWiring.php

* Note that, ideally, all information used to instantiate service objects should come
* from configuration. Information derived from the current request is acceptable, but
* only where there is no feasible alternative. It is preferred that such information
* (like the client IP, the acting user's identity, requested title, etc) be passed to
* the service object's methods as parameters. This makes the flow of information more
* obvious, and makes it easier to understand the behavior of services.

For example, to follow this pattern the SpecialPageFactory has a getPage function, but every caller has to call setContext on it to make sure for which context it is used/working. That also includes that the constructor cannot use the context, which is also sometimes violated (T297203). The service is not responsible for the context.

A similiar discussion happen on T249668: 'LinkRenderer' service violates to not inspect request/session

Umherirrender removed a subscriber: Umherirrender.Jan 2 2022, 8:43 PM

Tgr mentioned this in T314008: Promote dirty RequestContext::clearActionName to runtime warning.Nov 15 2022, 5:06 AM

Tgr mentioned this in T323184: SpecialPage transclusion triggers deprecated "Unexpected clearActionName after getActionName".Nov 16 2022, 3:04 AM

Daimona added a subscriber: Daimona.Nov 17 2022, 10:16 PM

Parser code for special page transclusion replaces main contextOpen, Needs TriagePublicBUG REPORTActions

Description

Related Objects

Event Timeline

Parser code for special page transclusion replaces main context
Open, Needs TriagePublicBUG REPORT
Actions