Page MenuHomePhabricator

Finalise addModuleStyles() and addModuleScripts() legacy behaviours
Closed, ResolvedPublic

Description

This follows-up on 80e5b160e0985, which removed the last use of these methods in core.

The addModuleScripts() and addModuleStyles() methods have very different histories and use cases, none
of which actually relate to the need to load only part of a module.

Both methods were originally introduced for the user and site modules, and have always been used in such a way that does load both the styles and scripts of the modules involved. The only reason they were loaded, in two parts, with these methods is not because of *what* the methods load, but because of *how* the methods load the specified code.

addModuleStyles()

This method is special because it loads styles using a <link rel=stylesheet> tag, instead of using JavaScript. As such, it is intended for styles that relate to skin and page content. As side-effect of this important loading method, if the module were to contain JavaScript code as well, it would be ignored. As of T87871 and T92459, doing so now produces a warning.

From T92459:

  • Detect the issue and trigger a debug log message. – a464d1d41d69
  • Fix known offenders in core, bundled extensions and wmf-production.
  • Promote log message from level=debug to level=warning. (Released in 1.29) – 976943c9913b842
  • Enforce the restriction with non-fatal error. (Released in 1.29) – ed28e106e366f

As it stands, we want to keep the method given it has important semantic differences that cannot be merged into addModules() without confusion. For example, loading a module as a stylesheet means it cannot have dependencies due to being hardcoded into the page. It also allows developer to retain control whether or not to load a module as a stylesheet. In most cases, style modules intended for JavaScript also have scripts. But it is possible for JS module to depend on a styles-only module that is only needed by JS, which should not be loaded as a blocking stylesheet. As such, we do not want to automatically load styles-only modules a blocking stylesheet via addModules(). Instead we'll keep using addModuleStyles() for that purpose.

addModuleScripts()

This method is special because, unlike regular module loading, this method will transport the script source to the client without closure wrapping, without mw.loader wrapping, without localStorage caching, and thus in the global scope. It's sole purpose was to allow loading of legacy user scripts in the global scope.

However, that use case has since been solved by other means. Leaving no further use of this system.

  • 1.32 Release:
    • Deprecate addModuleScripts() in OutputPage/ParserOutput.
  • 1.33 Release:
    • Remove addModuleScripts() in OutputPage/ParserOutput.
    • Remove setModuleScripts() in ResourceLoaderClientHtml. (already deprecated since 1.28)
    • Deprecate getModuleScripts() from OutputPage/ParserOutput. (no-op)
  • 1.34 Release:
    • Remove getModuleScripts() from OutputPage/ParserOutput.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Krinkle renamed this task from Remove support for addModuleStyles() and addModuleScripts() legacy behaviours to Finalise addModuleStyles() and addModuleScripts() legacy behaviours.Mar 2 2018, 1:46 AM

Change 415794 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Deprecate OutputPage::addModuleScripts()

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

Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.

OutputPage::addModuleScripts() is still used by GlobalCssJs.

@Legoktm Interesting. That means User:/global.js and MediaWiki:Global.js are evaluated as legacy scripts with global scope (e.g. var a = 1 creating window.a etc.).

I don't think we need that given that these are relatively new pages. We can proceed in one of two ways:

  1. Don't use legacy global evaluation anymore. This would match behaviour with gadgets, and with user-group scripts like MediaWiki:Group-user.js, which also don't use this anymore. Implementation: Change call to addModuleScripts to addModules().
  2. Keep legacy global evaluation. Implementation: Update ResourceLoaderGlobalModule to override getScript() in a way that calls super, and then wraps the script in a $.globalEval() call.

This last point matches what mediawiki.js in MediaWiki core already does the user module, which is the last remaining module still using global eval, and does so without using addModuleScripts.

Right now core actually does this by hardcoding things for user in two places, by transporting the code as a string (instead of a function closure) from load.php, and then the call to $.globalEval() is in mediawiki.js. But really I should update it to use the same method I just recommended above, which is to centralise this logic within the ResourceLoaderUserModule class.

Core: ResourceLoader.php/for-user/string, mediawiki.js/for-string/globalEval.

We've always documented this as not being global (c.f. https://www.mediawiki.org/wiki/Help:Extension:GlobalCssJs#Variables) even though it wasn't, so (given appropriate warning), I think we can stop using the legacy global evaluation.

Change 416593 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Add test for non-empty user module in scripts-only queue

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

Change 416593 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Add test for non-empty user module in scripts-only queue

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

Change 416619 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Support loading group=user modules with addModules()

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

Change 416624 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/GlobalCssJs@master] Use addModules() instead of addModuleScripts()

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

Change 416619 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Support loading group=user modules with addModules()

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

Change 416624 merged by jenkins-bot:
[mediawiki/extensions/GlobalCssJs@master] Use addModules() instead of addModuleScripts()

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

Now unblocked and ready for review:

[mediawiki/core@master] resourceloader: Deprecate OutputPage::addModuleScripts()
https://gerrit.wikimedia.org/r/415794

Change 415794 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Deprecate OutputPage::addModuleScripts()

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

Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)
Krinkle added a project: MW-1.33-release.

Change 440483 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Un-deprecate ClientHtml::setModuleStyles()

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

Change 440483 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Un-deprecate ClientHtml::setModuleStyles()

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

Change 494053 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/3D@master] Remove use of deprecated addModuleScripts()

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

Change 494057 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Collection@master] Remove references to deprecated 'modulescripts'

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

Change 494058 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Flow@master] Remove use of deprecated addModuleScripts()

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

Change 494059 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Wikibase@master] Remove use of deprecated addModuleScripts() method.

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

Change 494063 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Remove addModuleScripts, and deprecate getModuleScripts.

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

Krinkle changed the task status from Stalled to Open.Mar 3 2019, 7:20 PM
Krinkle claimed this task.

Change 494053 merged by jenkins-bot:
[mediawiki/extensions/3D@master] Remove use of deprecated addModuleScripts()

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

Change 494058 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Remove use of deprecated addModuleScripts()

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

Change 494059 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove use of deprecated addModuleScripts() method.

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

Change 494057 merged by jenkins-bot:
[mediawiki/extensions/Collection@master] Remove references to deprecated 'modulescripts'

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

Change 494512 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/GlobalUserPage@master] Remove use of deprecated addModuleScripts() method

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

Change 494512 merged by jenkins-bot:
[mediawiki/extensions/GlobalUserPage@master] Remove use of deprecated addModuleScripts() method

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

Change 494063 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Remove addModuleScripts, and deprecate getModuleScripts.

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

Krinkle removed a project: MW-1.33-release.

(Next and last step will be during the MW 34 cycle)

Change 509159 merged by jenkins-bot:
[mediawiki/core@master] Remove deprecated unused method getModuleScripts()

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

Krinkle updated the task description. (Show Details)