Page MenuHomePhabricator

Re-examine OutputPageScriptsForBottomQueue hook for extension-added user modules
Closed, ResolvedPublic

Description

Background: The GlobalCssJs extension adds global user modules, which were not invalidating the version properly (bug 62602). To fix this, a hook named OutputPageScriptsForBottomQueue was added to core (Ifccac7889e80) which allows extensions to define modules to be added directly to the bottom queue, bypassing the startup module.

Further testing of the hook as used by GlobalCssJs has come up with a few issues like bug 68521 (adding CSS/JS on Special:UserLogin), and bug 68547 (CSS not loaded for users with JS disabled).

The first bug is relatively easy to fix (I33f39a5), but the second is probably not.

The hook adds the modules using TYPE_COMBINED uncondionally, so it is impossible for extensions to add just CSS or just JS (addModuleStyles/addModuleScripts).

Convo between Krinkle & I in -dev: http://bots.wmflabs.org/~wm-bot/logs/%23wikimedia-dev/20140724.txt

Relevant parts:
[22:39:57] <legoktm> Krinkle: So, I'm thinking right now the OutputPageScriptsForBottomQueue hook needs some kind of differentation between styles/scripts like addModuleStyles/addModuleScripts that can be loaded separately
[22:40:18] <legoktm> wfRunHooks( 'OutputPageScriptsForBottomQueue', array( $this, &$combined, &$stylesonly, &$scriptsonly ) );
[22:40:21] <Krinkle> legoktm: Forget what I said earlier about them separate.
[22:40:33] <Krinkle> legoktm: Is there a reason we would actually want them to be separate?
[22:40:36] <Krinkle> (other than the if condition bug)
[22:40:46] <Krinkle> a benefit, user experience, end result.
[22:40:47] <legoktm> So that global.css will work if JS is disabled
[22:40:55] <Krinkle> Right, there we go.
[22:40:57] <Krinkle> OK.
[22:41:20] <Krinkle> You did say that now that I think back, but I misread it.
[22:41:46] <Krinkle> legoktm: Well, I'm actually thinking of getting rid of it alltogether.
[22:41:57] <Krinkle> I'd much rather only have addModules() (and *Styles, *Scripts)
[22:42:12] <legoktm> getting rid of the hook?
[22:42:13] <Krinkle> and have it output mw.loader.load or <link>/<script> accordingly.
[22:42:39] <Krinkle> Well, implicitly yes, but the bigger picture of getting rid of addRLink as the entry point.
[22:43:10] <Krinkle> So your extension would just do what it always did: $out->addModules('ext.global.user') or whatev.
[22:43:26] <Krinkle> and OutputPage should know whether the module is private/user and needs certain treatment.
[22:43:56] <Krinkle> Or in this case you'd call addModuleStyles/Script from OutputPage and it should know to include a version number and username if it's user specific.
[22:44:04] <Krinkle> Most of this logic is in addRLink actually
[22:44:26] <Krinkle> Except that it only handles with the edge cases
[22:44:31] <legoktm> yeah, that seems doable I think
[22:44:34] <Krinkle> and defaults to <script src="load.php">
[22:44:41] <Krinkle> instead of <scritp>mw.loader.load(Array)
[22:45:05] <Krinkle> legoktm: I don't want to block GlobalJsCss too long though.
[22:45:29] <Krinkle> Can you maybe see if this is relatively straight forward? I have a feeling the infrastructure is almost there, just out of reach.
[22:46:12] <Krinkle> If it's too much, we can put it on the backlog and go forward. Though I guess at this point it's not a question of keeping it as-is because as-is it is broken. I forgot that for a sec.
[22:47:03] <legoktm> So if I understand you correctly, you want to basically eliminate the special case handling in OutputPage::getScriptsForBottomQueue for the user/site modules, and have them be split out in makeResourceLoaderLink instead?
[22:47:08] <Krinkle> So we'll need to change the hook either way. In that case, with this prospect, I'd like for the hook not to go out in a stable release if we're phasing that entire logic out in a few weeks.
[22:47:45] <Krinkle> legoktm: Basically right now (up until last week when I merged your new hook) the only way to addmodules was addmodules() and its variants.
[22:48:13] <Krinkle> And all is well until you need separate requests (for global.js), or versioned/user specific urls, or froeign sources.
[22:48:33] <Krinkle> makeResourceLoaderLink handles all those cases and more, except that it doesns't handle the plain case of addModules().
[22:49:09] <Krinkle> I'd like for addModules() to remain the main interface, and OutputPage recognise the cases for makeResourceLoaderLink and spit out that kind of link when it needs to, leaving the rest in a regular load(Array) call.
[22:49:19] <Krinkle> legoktm: The main hurdle will be meta data.
[22:49:39] <Krinkle> Right now all meta data is available in modules for makeResourceLoaderLink, except for 1 property: The fact that there is something "strange".
[22:50:08] <Krinkle> Once you give a module to makeResourceLoaderLink it does something "special", once it knows a module it special it knows what to do, but right now it's hard to know that something is special.
[22:50:24] <legoktm> I guess we could add something like 'special' => true to the module registration? :P
[22:52:19] <Krinkle> It's only the fact that it is user specific.
[22:53:06] <Krinkle> and then there is 'site' which isn't user specific, has a reilable version number in the registry, is loadable via mw.loader, but we want it separate because of legacy global scope.
[22:53:47] <Krinkle> I guess we can use addModuleScripts for that
[22:53:53] <Krinkle> It does that already

Roan: Krinkle was going to discuss ^ with you "tomorrow" (yesterday now), did that ever happen?

I think we can just special case the "user" group, and load each of those modules in their own request. GlobalCssJs (and even core!) would then just use $out->addModule/Styles/Scripts and OutputPage would take care of it. I'll upload a POC patch that does that.


Version: 1.24rc
Severity: normal

Details

Reference
bz68712

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:26 AM
bzimport set Reference to bz68712.
bzimport added a subscriber: Unknown Object (MLST).

Change 149806 had a related patch set uploaded by Legoktm:
Load "user" group modules in individual requests

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

Change 150445 had a related patch set uploaded by Legoktm:
Revert "Add OutputPageScriptsForBottomQueue hook"

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

Change 149806 abandoned by Legoktm:
Load "user" group modules in individual requests

Reason:
Not necessary.

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

Change 150445 merged by jenkins-bot:
Revert "Add OutputPageScriptsForBottomQueue hook"

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

I discussed this with Roan on IRC today. He pointed out that if we use addModuleStyles/addModuleScripts, the modules go through OutputPage::makeResourceLoaderLink, resulting in independent <link>/<script> tags as we want, while addModules just adds them to a mw.loader.load array. Additionally, OutputPage already special cases the 'user' group versioning, so that's taken care of.

Since we no longer need the hook, I've reverted it, and updated GlobalCssJs as well. Yay!