jenkins-bot has submitted this change and it was merged.

Change subject: resourceloader: Move 'site' and 'user' logic to 
makeModuleResponse
......................................................................


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
---
M includes/resourceloader/ResourceLoader.php
M resources/src/mediawiki/mediawiki.js
M tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
3 files changed, 35 insertions(+), 34 deletions(-)

Approvals:
  Aaron Schulz: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/resourceloader/ResourceLoader.php 
b/includes/resourceloader/ResourceLoader.php
index 97a86c3..34a0a99 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -1031,9 +1031,23 @@
                                                $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 @@
         * 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 @@
         * @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 db5a4fb..04807f4 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 9a3e222..c24a321 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -281,7 +281,6 @@
                                'name' => 'test.example',
                                'scripts' => [],
                                'styles' => [ 'css' => [ '.mw-example {}' ] ],
-                               'messages' => new XmlJsCode( '{}' ),
 
                                'expected' => 'mw.loader.implement( 
"test.example", [], {
     "css": [
@@ -320,17 +319,9 @@
 
                                '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 @@
         */
        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 @@
         */
        public function testMakeLoaderImplementScriptInvalid() {
                $this->setExpectedException( 'MWException', 'Invalid scripts 
error' );
-               ResourceLoader::makeLoaderImplementScript(
+               $rl = TestingAccessWrapper::newFromClass( 'ResourceLoader' );
+               $rl->makeLoaderImplementScript(
                        'test', // name
                        123, // scripts
                        null, // styles

-- 
To view, visit https://gerrit.wikimedia.org/r/310727
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I527d01926fb6e4ce68c931695d830cdb9ceb608c
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: Legoktm <legoktm.wikipe...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to