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 <https://gist.github.com/Krinkle/5db1d237da241b243485>.

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

diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 1c76f0b..e2caf80 100644 (file)
--- 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 305b197..7875048 100644 (file)
--- 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 700513e..7825f22 100644 (file)
--- 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