Page MenuHomePhabricator

ResourceLoader should use context/config instead of global variables
Open, LowPublic

Description

Currently, ResourceLoader modules use global config variables; they should rather get them from the local Config object.


  • ResourceLoader.php:
    • $wgShowExceptionDetails in static formatException().
    • $wgResourceLoaderDebug and $wgRequest in static inDebugMode().
    • Call to OutputPage::transformCssMedia(), which in turn uses $wgRequest.
  • ResourceLoaderContext.php
    • $wgLanguageCode in getLanguage() as fallback.
  • ResourceLoaderFileModule.php: $IP, $wgResourceBasePath, $wgExtensionAssetsPath.
  • ResourceLoaderImage.php: $wgSVGConverter, $wgSVGConverterPath.
  • ResourceLoaderModule.php: $wgContLang in getFlip().
  • ResourceLoaderStartUpModule.php:
    • $wgContLang in getConfigSettings().
    • $wgIncludeLegacyJavaScript in getLegacyModules(). - T130879

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:16 AM
bzimport set Reference to bz34738.
bzimport added a subscriber: Unknown Object (MLST).

$wgUser is used in ResourceLoaderUserCSSPrefsModule.php, ResourceLoaderUserOptionsModule.php and ResourceLoaderUserTokensModule.php.
$wgLang is used in ResourceLoaderContext.php.
$wgRequest is used in ResourceLoader.php.

These variables should be get from a context object instead (not the ResourceLoader one, but RequestContext or ContextSource).

Krinkle added a project: Technical-Debt.
Krinkle set Security to None.
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.
Krinkle removed a subscriber: Unknown Object (MLST).

Most of the mentioned use of globals have been mitigated. A few other ones exist still, though:

ack '$wg' -Q --php includes/resourceloader/

Added results to the task description.

Krinkle renamed this task from ResourceLoader should have a context instead of using global object variables to ResourceLoader should use a context/config instead of global variables.Feb 10 2019, 7:48 PM
Krinkle renamed this task from ResourceLoader should use a context/config instead of global variables to ResourceLoader should use context/config instead of global variables.
Krinkle added a project: good first task.
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/815776/ would alleviate the 'usage of $wgRequest global' concern, but a second pair of eyes is needed:

  • Since [\MediaWiki\ResourceLoader\ResourceLoader]::inDebugMode() is a static function with neither WebRequest exposed via $this->getRequest() or [\MediaWiki\ResourceLoader\Context]->getRequest() available. Unless extensions can/should fetch a 'ResourceLoader object' (and therefore convert this function into a non-static one), I'm not sure how to refrain from this "last restort".
  • Apart from OutputPage and OutputPageTest, [\MediaWiki\ResourceLoader\ResourceLoader]::makeCombinedStyles() is the only caller for OutputPage::transformCssMedia(), hence I couldn't refrain from using RequestContext::getMain() here either.
  • According to T165176, RequestContext::getMain() can cause side-effects? Will that affect the ResourceLoader Context too?

Change 815776 had a related patch set uploaded (by Southparkfan; author: Southparkfan):

[mediawiki/core@master] ResourceLoader: replace $wgRequest global with RequestContext

https://gerrit.wikimedia.org/r/815776

Change 817405 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/TemplateStyles@master] Hooks: Remove use of unsafe inDebugMode() from handleTag()

https://gerrit.wikimedia.org/r/817405

@Southparkfan Thanks. Your patch avoids use of $wgRequest which unblocks removal of that variable from core. However, this task represents the work for T32956 and would not be resolved unless other forms of global state are avoided as well. Whether we obtain WebRequest from $wgRequest or RequestContext::getMain() is a minor detail from this task's perspective.

I'm happy to accept it, but keep the task and checkbox open after this lands.

If you'd like to resolve the use of global WebRequest in ::inDebugMode(), I can recommend two approaches to try.

  1. Perhaps create a setDebugMode() method marked @internal that we'd call from load.php and possibly OutputPage.php. It could take WebRequest and Config as parameter. Doing this would actually highlight an issue which is that we appear to be reading the cookie even when on load.php which is potentially a problem even today. The cookie should instruct OutputPage to make load.php?debug=true requests, there is no need for it to read it directly. If it does, it might actually poison the cache.
  1. Deprecate it and phase out callers as much as possible. I've done this for one special case already (patch above), but there are a handful more. Codesearch: inDebugMode
    • Score/ScoreVeResourceLoaderModule.php: This can use $context->getDebug() instead, or even better remove it in favour of $context->encodeJson(). See other callers to encodeJson for examples.
    • Citoid/CitoidDataModule.php: Same as Score.
    • AdvancedSearch/SearchnamespaceTokenModule.php: Same as Score.
    • mediawiki-core/SpecialJavaScriptTest.php: This can probably use getRequest()->getRawVal('debug') instead. It doesn't need to support the cookie or config, and also doesn't need to parse the value as it is passed to RL\Context which will take care of that instead.

@Southparkfan Thanks. Your patch avoids use of $wgRequest which unblocks removal of that variable from core. However, this task represents the work for T32956 and would not be resolved unless other forms of global state are avoided as well. Whether we obtain WebRequest from $wgRequest or RequestContext::getMain() is a minor detail from this task's perspective.

I understand, eliminating $wgRequest was low-hanging fruit here. DI is still better than either using ::getMain().

I'm happy to accept it, but keep the task and checkbox open after this lands.

If you'd like to resolve the use of global WebRequest in ::inDebugMode(), I can recommend two approaches to try.

Now that we're at it, I'll try to get a durable solution; after all, I would like to reduce technical debt, not move it somewhere else. Assistance is needed to get me starting here, though :-). Your advice is welcome.

  1. Perhaps create a setDebugMode() method marked @internal that we'd call from load.php and possibly OutputPage.php. It could take WebRequest and Config as parameter. Doing this would actually highlight an issue which is that we appear to be reading the cookie even when on load.php which is potentially a problem even today. The cookie should instruct OutputPage to make load.php?debug=true requests, there is no need for it to read it directly. If it does, it might actually poison the cache.

Assumptions

  • load.php (RL\Context) only needs to know if ?debug=<value> exists; cookies and config don't matter here.
  • index.php emits HTML elements (sourcing resources from load.php) that may or may not contain ?debug, depending on the sixth argument of ResourceLoader::makeLoaderQuery(). This entry point is not interested in the presence of a ?debug parameter - inverse of load.php.

Future situation
Is the setDebugMode() (I assume you insist on a new method in RL\RL, not in RL\Context, correct?) method something that should be accompanied by a getDebugMode() that returns any of RL\Context::DEBUG_{OFF,LEGACY,MAIN} (if inside RL class) or {true,false}?

These are core callers of ::inDebugMode():

  • OutputPage->getRlClientContext() (private): uses ::inDebugMode() to craft a query (ResourceLoader::makeLoaderQuery()), which is used in new RL\Context( <ResourceLoader>, FauxRequest( $query ) ). Using RL\Context->getDebug() is not possible here due to a catch-22, but WebRequest and Config are accessible in this class. You could migrate the 'has config/contains cookie' check to here without using MediaWikiServices::getInstance()->getMainConfig() and RequestContext::getMain().
  • SpecialJavaScriptTest->exportJS(): same as above, the result of ::inDebugMode() is needed before an instance of RL\Context is constructed, but Config and WebRequest are accessible.
  • SpecialJavaScriptTest->renderPage(): no RL\Context to be found here, Config and WebRequest are accessible.
  • ResourceLoader::encodeJsonForScript() (internal): calls self::inDebugMode(). Callers are ResourceLoader::makeMessageSetScript() and ResourceLoader::makeConfigSetScript(). I don't see callers for ::makeMessageSetScript(), ::makeConfigSetScript() has multiple callers, some of which do have RL\Context available, but I don't see how that can help us here.

Except for ::encodeJsonForScript(), duplicating the 'has config/contains cookie' logic is possible and this will remove the need for ::inDebugMode(). However, dplication of code is not ideal and can be abstracted away to $this->getOutput()->getResourceLoader()->getDebugMode().

  1. Deprecate it and phase out callers as much as possible. I've done this for one special case already (patch above), but there are a handful more. Codesearch: inDebugMode
    • Score/ScoreVeResourceLoaderModule.php: This can use $context->getDebug() instead, or even better remove it in favour of $context->encodeJson(). See other callers to encodeJson for examples.
    • Citoid/CitoidDataModule.php: Same as Score.
    • AdvancedSearch/SearchnamespaceTokenModule.php: Same as Score.
    • mediawiki-core/SpecialJavaScriptTest.php: This can probably use getRequest()->getRawVal('debug') instead. It doesn't need to support the cookie or config, and also doesn't need to parse the value as it is passed to RL\Context which will take care of that instead.

Thanks!

Change 817405 merged by jenkins-bot:

[mediawiki/extensions/TemplateStyles@master] Hooks: Remove use of unsafe inDebugMode() from handleTag()

https://gerrit.wikimedia.org/r/817405