Page MenuHomePhabricator

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

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.

Event Timeline

Tgr added a subscriber: daniel.

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

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.

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

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