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.

High level

  • ResourceLoader.php:
    • $wgShowExceptionDetails in static formatException().
    • $wgResourceLoaderDebug and $wgRequest in static inDebugMode(). — can't be improved without breaking changes and alternatives exist already. migrate and remove instead of improving (see below).
  • 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

ResourceLoader::inDebugMode

Context:

Whether ResourceLoader acts in debug mode is determined by the ?debug parameter to load.php. This is handled in the RL\Context class and accepts values 0, 1, 2, true, false. There is also a frontend to this via wiki page views by the same`?debug` parameter e.g. on /wiki/Apple?debug=2 or /w/index.php?debug=2. This is handled by OutputPage and then passed on to RL\ClientHtml which then passes it to load.php.

Developers can make their debug session sticky for a short period of time by setting the resourceLoaderDebug cookie and/or by setting $wgResourceLoaderDebug = in LocalSettings.php locally. This is similarly handled by OutputPage, and then passed to load.php. That is - load.php should be stateless and directly be aware of any MW settings or cookies.

The main reason this method exists is because debug mode decides whether to minify code or not. As such, we historically used ResourceLoader::inDebugMode() to decide between minified and raw chunks. We now handle this differently, via RL\Context::encodeJson() which automatically decides between pretty for debug or minified for prod. If the debug settings is still needed, it can be accesed from RL\Context::getDebug(). This means information is parsed centrally and passed down to where it is needed via the RL Context object.

Older code that still uses the global static ResourceLoader::inDebugMode method:

  • ResourceLoader::encodeJsonForScript() Deprecate and remove once unused. We've removed most use of these but we have a utility functions calling them from older code:
    • ResourceLoader::makeConfigSetScript Deprecate and remove once unused. 4 extension remaining:
      • Used in mediawiki/extensions/UniversalLanguageSelector. Migrate to $context->encodeJson().
      • Used in mediawiki/extensions/Kartographer. Migrate to $context->encodeJson().
      • Used in mediawiki/extensions/CodeMirror. Migrate to Package files as virtual JSON file.
      • Used in mediawiki/extensions/Wikibase. Migrate to $context->encodeJson().
    • ResourceLoader::makeInlineCodeWithModule - This formats a single array of strings. It doesn't need to vary by debug mode, and can call json_encode directly instead.
  • OutputPage. It uses inDebugMode to configure RL\ClientHtml and to parse the cookie/MW config/requets param for debug mode. This is the original purpose and remains needed. Everything else flows down from here. There is no need for any other code to consider cookies or MW configuration, because it would get it from here. I suggest we copy ResourceLoader::inDebugMode to private OutputPage method, refactor to use local context, and then use call for RL\ClientHtml instead.
  • SpecialJavaScriptTest. It uses inDebugMode() enable debug mode automatically based on cookie or MW config. No longer needed after source maps (T47514). It can be reduced to WebRequest->getRawVal('debug') ?? '0' so that you can still manually enable ?debug=2 here.

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

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

[mediawiki/core@master] ResourceLoader: Remove 1/2 inDebugMode() calls in SpecialJavaScriptTest

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

Change 907514 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Remove 1/2 inDebugMode() calls in SpecialJavaScriptTest

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

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

[mediawiki/extensions/WikibaseLexeme@master] Avoid global ResourceLoader::inDebugMode() in LexemeLanguageCodePropertyIdConfig

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

Change 947983 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Avoid global ResourceLoader::inDebugMode() in LexemeLanguageCodePropertyIdConfig

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

From the task description:
  • $wgResourceLoaderDebug and $wgRequest in static inDebugMode().

This is the last remaining item in the current task scope. Due to the static nature of the method, there isn't really a way to improve it in its current state.

When auditing its callers, I found that the majority of callers due to need to know about "debug mode" for its own sake, but rather passed it along like Xml::encodeJsCall( …, inDebugMode() ), which creates a snippet of JSON where the debug parameter controls whether to minify the snippet or to pretty-print it. I migrated most of these to use the dedicated RL\Context::encodeJson() method which takes care of this already.

In other cases, I have either:

  • removed it in favour of always compressing, i.e. when deeming the data sufficiently hidden far away from RL as to not benefit (or even reasonaly discover) a cache-busting pretty-printing ability. For example, in TemplateStyles.
  • removed it in favour of always pretty-printing, i.e. when a higher level can take care of the minification instead.

In many cases, other commits have since then removed these callers entirely in favour of packageFiles. For example: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/916063 after https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/738006.

What's left is:

  1. OutputPage: to construct RL\ClientHtml and its context based on page view request cookie/param/config. This is the only case afaik where we need to involve those things. Everything else flows from here. I.e. nothing else needs to honour the cookie or request parameter since it's going to be generated by or nested within, or in response to a separate URL created by, that ClientHtml context. I suggest we copy it to private instance method and call that from the 1 place in OutputPage instead.
  2. JavaScriptTest: to enable debug mode. No longer needed after source maps I think. Can be removed, or reduced to WebRequest->getRawVal('debug') ?? '0'.
  3. Indirect calls via encodeJsonForScript from static utility methods like makeConfigSetScript, makeInlineScript, makeInlineCodeWithModule. I suggest we fix a few prominent indirect callers in the same way describribed above (replace with local $context->encodeJson, or with virtual files or package files). Then, update the remaining static utility methods to always minify and remove use of inDebugMode. It's functionally identical.

After that, inDebugMode can be hard-deprecated and subsequently removed.

Adding to the pool for potential next tasks to work on in the ResourceLoader area after the other tasks are finished.

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

[mediawiki/core@master] debug: Remove use of makeConfigSetScript() from MWDebug

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

Change #1084156 merged by jenkins-bot:

[mediawiki/core@master] debug: Remove use of makeConfigSetScript() from MWDebug

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

Change #1099033 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/extensions/Wikibase@master] use context instead of global variables

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

Change #1099017 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] SpecialJavaScriptTest: ResourceLoader should use context/config instead of global variables

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

Change #1099037 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/extensions/Kartographer@master] use context instead of global variables

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

Change #1099040 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/extensions/UniversalLanguageSelector@master] UniversalLanguageSelector: ResourceLoader should use context/config instead of global variables

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

Quote:

makeConfigSetScript Deprecate and remove […] Migrate to $context->encodeJson().

This appears to be a mistake. These are two very different methods that do two very different things. Maybe you meant $context->addJsConfigVars?

Change #1099017 merged by jenkins-bot:

[mediawiki/core@master] SpecialJavaScriptTest: Pass `debug` from current req instead of global variables

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

Change #1105376 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] ResourceLoader: Deprecate ResourceLoader::makeConfigSetScript

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