From: Timo Tijhof <krinklemail@gmail.com>
Date: Thu, 15 Sep 2016 04:01:09 +0000 (-0700)
Subject: resourceloader: Move 'site' and 'user' logic to makeModuleResponse
X-Git-Tag: 1.31.0-rc.0~5521^2
X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_ecrire%28%22upgrade%22%2C%22reinstall=non%22%29%20.%20%22?a=commitdiff_plain;h=09e1b91756686bb55dc1521d9f53b6ef23a09c01;p=lhc%2Fweb%2Fwiklou.git

resourceloader: Move 'site' and 'user' logic to makeModuleResponse

* Keep this out of makeLoaderImplementScript() to keep it more generic
  and to simplify future refactoring.

* Remove now-broken test case that asserted that the output varies
  by global debug mode.

* Make the test responsible for wrapping in XmlJsCode. Previously
  this magically happened because the module name was "user" in the
  last test case.

* Make makeLoaderImplementScript protected. It's not used anywhere
  outside ResourceLoader and we should keep it that way.

Test plan:
* Verify output unchanged:
  - load.php?modules=user&only=scripts&user=Admin (raw code)
  - load.php?modules=user&user=Admin (implement with unwrapped string)
  - load.php?modules=jquery.client (implement with closure)

Change-Id: I527d01926fb6e4ce68c931695d830cdb9ceb608c
---

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index 97a86c3a0b..34a0a99d31 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -1031,9 +1031,23 @@ MESSAGE;
 						$strContent = isset( $styles['css'] ) ? implode( '', $styles['css'] ) : '';
 						break;
 					default:
+						$scripts = isset( $content['scripts'] ) ? $content['scripts'] : '';
+						if ( is_string( $scripts ) ) {
+							if ( $name === 'site' || $name === 'user' ) {
+								// Legacy scripts that run in the global scope without a closure.
+								// mw.loader.implement will use globalEval if scripts is a string.
+								// Minify manually here, because general response minification is
+								// not effective due it being a string literal, not a function.
+								if ( !ResourceLoader::inDebugMode() ) {
+									$scripts = self::filter( 'minify-js', $scripts ); // T107377
+								}
+							} else {
+								$scripts = new XmlJsCode( $scripts );
+							}
+						}
 						$strContent = self::makeLoaderImplementScript(
 							$name,
-							isset( $content['scripts'] ) ? $content['scripts'] : '',
+							$scripts,
 							isset( $content['styles'] ) ? $content['styles'] : [],
 							isset( $content['messagesBlob'] ) ? new XmlJsCode( $content['messagesBlob'] ) : [],
 							isset( $content['templates'] ) ? $content['templates'] : []
@@ -1112,7 +1126,8 @@ MESSAGE;
 	 * Return JS code that calls mw.loader.implement with given module properties.
 	 *
 	 * @param string $name Module name
-	 * @param mixed $scripts List of URLs to JavaScript files or String of JavaScript code
+	 * @param XmlJsCode|array|string $scripts Code as XmlJsCode (to be wrapped in a closure),
+	 *  list of URLs to JavaScript files, or a string of JavaScript for `$.globalEval`.
 	 * @param mixed $styles Array of CSS strings keyed by media type, or an array of lists of URLs
 	 *   to CSS files keyed by media type
 	 * @param mixed $messages List of messages associated with this module. May either be an
@@ -1123,22 +1138,13 @@ MESSAGE;
 	 * @throws MWException
 	 * @return string
 	 */
-	public static function makeLoaderImplementScript(
+	protected static function makeLoaderImplementScript(
 		$name, $scripts, $styles, $messages, $templates
 	) {
-		if ( is_string( $scripts ) ) {
-			// 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' || $name === 'user' ) {
-				// Minify manually because the general makeModuleResponse() minification won't be
-				// effective here due to the script being a string instead of a function. (T107377)
-				if ( !ResourceLoader::inDebugMode() ) {
-					$scripts = self::filter( 'minify-js', $scripts );
-				}
-			} else {
-				$scripts = new XmlJsCode( "function ( $, jQuery, require, module ) {\n{$scripts}\n}" );
-			}
-		} elseif ( !is_array( $scripts ) ) {
+
+		if ( $scripts instanceof XmlJsCode ) {
+			$scripts = new XmlJsCode( "function ( $, jQuery, require, module ) {\n{$scripts->value}\n}" );
+		} elseif ( !is_string( $scripts ) && !is_array( $scripts ) ) {
 			throw new MWException( 'Invalid scripts error. Array of URLs or string of code expected.' );
 		}
 		// mw.loader.implement requires 'styles', 'messages' and 'templates' to be objects (not
diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index db5a4fb4d0..04807f4e4f 100644
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1879,8 +1879,8 @@
 				 * response contain calls to this function.
 				 *
 				 * @param {string} module Name of module
-				 * @param {Function|Array} [script] Function with module code or Array of URLs to
-				 *  be used as the src attribute of a new `<script>` tag.
+				 * @param {Function|Array|string} [script] Function with module code, list of URLs
+				 *  to load via `<script src>`, or string of module code for `$.globalEval()`.
 				 * @param {Object} [style] Should follow one of the following patterns:
 				 *
 				 *     { "css": [css, ..] }
diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
index 9a3e222622..c24a3211e7 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -281,7 +281,6 @@ mw.example();
 				'name' => 'test.example',
 				'scripts' => [],
 				'styles' => [ 'css' => [ '.mw-example {}' ] ],
-				'messages' => new XmlJsCode( '{}' ),
 
 				'expected' => 'mw.loader.implement( "test.example", [], {
     "css": [
@@ -320,18 +319,10 @@ mw.example();
 
 				'name' => 'user',
 				'scripts' => 'mw.example( 1 );',
+				'wrap' => false,
 
 				'expected' => 'mw.loader.implement( "user", "mw.example( 1 );" );',
 			] ],
-			[ [
-				'title' => 'Implement unwrapped user script',
-				'debug' => false,
-
-				'name' => 'user',
-				'scripts' => 'mw.example( 1 );',
-
-				'expected' => 'mw.loader.implement("user","mw.example(1);");',
-			] ],
 		];
 	}
 
@@ -342,17 +333,20 @@ mw.example();
 	 */
 	public function testMakeLoaderImplementScript( $case ) {
 		$case += [
-			'styles' => [], 'templates' => [], 'messages' => new XmlJsCode( '{}' ),
-			'debug' => true
+			'wrap' => true,
+			'styles' => [], 'templates' => [], 'messages' => new XmlJsCode( '{}' )
 		];
 		ResourceLoader::clearCache();
-		$this->setMwGlobals( 'wgResourceLoaderDebug', $case['debug'] );
+		$this->setMwGlobals( 'wgResourceLoaderDebug', true );
 
+		$rl = TestingAccessWrapper::newFromClass( 'ResourceLoader' );
 		$this->assertEquals(
 			$case['expected'],
-			ResourceLoader::makeLoaderImplementScript(
+			$rl->makeLoaderImplementScript(
 				$case['name'],
-				$case['scripts'],
+				( $case['wrap'] && is_string( $case['scripts'] ) )
+					? new XmlJsCode( $case['scripts'] )
+					: $case['scripts'],
 				$case['styles'],
 				$case['messages'],
 				$case['templates']
@@ -365,7 +359,8 @@ mw.example();
 	 */
 	public function testMakeLoaderImplementScriptInvalid() {
 		$this->setExpectedException( 'MWException', 'Invalid scripts error' );
-		ResourceLoader::makeLoaderImplementScript(
+		$rl = TestingAccessWrapper::newFromClass( 'ResourceLoader' );
+		$rl->makeLoaderImplementScript(
 			'test', // name
 			123, // scripts
 			null, // styles