From afce927c8fad3017127438950f2d84b60f18a547 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 29 Jul 2015 16:38:28 -0700 Subject: [PATCH] resourceloader: Ensure 'user' loads after 'site' (asynchronously) Regression from 19a40cd3ad which made the 'site' module load asynchronously, but the 'user' module was still loaded synchronously which meant it ran before the site module finished. Full test script at . Also: * This changes the 'user' module to load asynchronously. * Similar to 19a40cd3ad for site module, this makes the styles for the user module load twice. Harmless but doesn't look pretty internally. * Remove the obsolete XXX-comment from 0b5389d98d (r56770). * Add comment documenting the fact that the 'excludepages' feature can cause User/common.js and User/vector.js to be mis-ordered when the user previews common.js edits. This has always been the case (since 2009) and is merely being documented here. Bug: T32358 Bug: T106736 Bug: T102077 Change-Id: Id599b6be42613529fb7f4dd3273f36ccadb3a09e --- includes/OutputPage.php | 30 ++++++++++++++++------ includes/resourceloader/ResourceLoader.php | 4 +-- resources/src/mediawiki/mediawiki.js | 22 +++++++++++----- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/includes/OutputPage.php b/includes/OutputPage.php index 1c76f0bc41..e2caf80df6 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -3067,28 +3067,42 @@ class OutputPage extends ContextSource { $links[] = "\n" . $this->mScripts; // Add user JS if enabled + // This must use TYPE_COMBINED instead of only=scripts so that its request is handled by + // mw.loader.implement() which ensures that execution is scheduled after the "site" module. if ( $this->getConfig()->get( 'AllowUserJs' ) && $this->getUser()->isLoggedIn() && $this->getTitle() && $this->getTitle()->isJsSubpage() && $this->userCanPreview() ) { - # XXX: additional security check/prompt? - // We're on a preview of a JS subpage - // Exclude this page from the user module in case it's in there (bug 26283) - $links[] = $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS, false, + // We're on a preview of a JS subpage. Exclude this page from the user module (T28283) + // and include the draft contents as a raw script instead. + $links[] = $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_COMBINED, false, array( 'excludepage' => $this->getTitle()->getPrefixedDBkey() ), $inHead ); // Load the previewed JS - $links[] = Html::inlineScript( "\n" - . $this->getRequest()->getText( 'wpTextbox1' ) . "\n" ) . "\n"; + $links[] = ResourceLoader::makeInlineScript( + Xml::encodeJsCall( 'mw.loader.using', array( + array( 'user', 'site' ), + new XmlJsCode( + 'function () {' + . Xml::encodeJsCall( '$.globalEval', array( + $this->getRequest()->getText( 'wpTextbox1' ) + ) ) + . '}' + ) + ) ) + ); // FIXME: If the user is previewing, say, ./vector.js, his ./common.js will be loaded // asynchronously and may arrive *after* the inline script here. So the previewed code - // may execute before ./common.js runs. Normally, ./common.js runs before ./vector.js... + // may execute before ./common.js runs. Normally, ./common.js runs before ./vector.js. + // Similarly, when previewing ./common.js and the user module does arrive first, it will + // arrive without common.js and the inline script runs after. Thus running common after + // the excluded subpage. } else { // Include the user module normally, i.e., raw to avoid it being wrapped in a closure. - $links[] = $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS, + $links[] = $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_COMBINED, /* $useESI = */ false, /* $extraQuery = */ array(), /* $loadCall = */ $inHead ); } diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 305b197d03..7875048c86 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -1105,9 +1105,9 @@ MESSAGE; $name, $scripts, $styles, $messages, $templates ) { if ( is_string( $scripts ) ) { - // Site module is a legacy script that runs in the global scope (no closure). + // Site and user module are a legacy scripts that run in the global scope (no closure). // Transportation as string instructs mw.loader.implement to use globalEval. - if ( $name !== 'site' ) { + if ( $name !== 'site' && $name !== 'user' ) { $scripts = new XmlJsCode( "function ( $, jQuery ) {\n{$scripts}\n}" ); } } elseif ( !is_array( $scripts ) ) { diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index 700513e4ee..7825f22bf0 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -1219,12 +1219,22 @@ script( $, $ ); handlePending( module ); } else if ( typeof script === 'string' ) { - // Site module is a legacy script that runs in the global scope. This is transported - // as a string instead of a function to avoid needing to use string manipulation to - // undo the function wrapper. - registry[module].state = 'ready'; - $.globalEval( script ); - handlePending( module ); + // Site and user modules are a legacy scripts that run in the global scope. + // This is transported as a string instead of a function to avoid needing + // to use string manipulation to undo the function wrapper. + if ( module === 'user' ) { + // Implicit dependency on the site module. Not real dependency because + // it should run after 'site' regardless of whether it succeeds or fails. + mw.loader.using( 'site' ).always( function () { + registry[module].state = 'ready'; + $.globalEval( script ); + handlePending( module ); + } ); + } else { + registry[module].state = 'ready'; + $.globalEval( script ); + handlePending( module ); + } } } catch ( e ) { // This needs to NOT use mw.log because these errors are common in production mode -- 2.20.1