Can you be more specific?

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

release is imminent.

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.

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.

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

In T36738#8106918, @Krinkle wrote:

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

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