https://bugzilla.wikimedia.org/show_bug.cgi?id=68712
Bug ID: 68712
Summary: Re-examine OutputPageScriptsForBottomQueue hook for
extension-added user modules
Product: MediaWiki
Version: 1.24-git
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: Unprioritized
Component: ResourceLoader
Assignee: [email protected]
Reporter: [email protected]
CC: [email protected], [email protected],
[email protected], [email protected],
[email protected]
Web browser: ---
Mobile Platform: ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
You are on the CC list for the bug.
_______________________________________________
Wikibugs-l mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
