Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/159086
Change subject: resourceloader: Do condition wrap in HTML instead of JS response
......................................................................
resourceloader: Do condition wrap in HTML instead of JS response
Follows-up 9272bc6c47, 03c503da22, 1e063f6078.
One can't wrap arbitrary javascript in an if-statement and have
its inner-body mean exactly the same.
Certain statements are only allowed in the top of a scope (such
as hoisted function declarations). These are not allowed inside
a block. They're fine in both global scope and local function scope,
but not inside an if-block of any scope.
The ECMAScript spec only states what is an allowed token, not
what is disallowed. Chrome's implementation (V8) allows it anyway
and hoists them to *outside* the condition. Firefox's (SpiderMonkey)
silently ignores the statement. Neither throw a SyntaxError for
unexpected token.
Our regular ResourceLoader responses only contain
mw.loader.implement() and mw.loader.state() call which could be
wrapped any issue. However such responses don't need wrapping as
they are only supported if made by mediawiki.js (in which case mw
is obviously loaded). The wrapping is only for legacy scripts
that execute in the global scope.
For those, let's wrap the script tag itself instead. That seems
like the most water-tight and semantically correct solution.
Had to bring in $isRaw from ResourceLoader.php, else the startup
module would have been wrapped as well (added regression test).
Bug: 69924
Change-Id: Iedda0464f734ba5f7a884726487f6c7e07d444f1
---
M includes/OutputPage.php
M includes/resourceloader/ResourceLoader.php
M tests/phpunit/ResourceLoaderTestCase.php
M tests/phpunit/includes/OutputPageTest.php
4 files changed, 40 insertions(+), 20 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/86/159086/1
diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index af90ca6..22a6012 100644
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -2769,7 +2769,10 @@
);
$context = new ResourceLoaderContext(
$resourceLoader, new FauxRequest( $query ) );
- // Extract modules that know they're empty
+
+ // Extract modules that know they're empty and
see if we have one or more
+ // raw modules
+ $isRaw = false;
foreach ( $grpModules as $key => $module ) {
// Inline empty modules: since they're
empty, just mark them as 'ready' (bug 46857)
// If we're only getting the styles, we
don't need to do anything for empty modules.
@@ -2779,6 +2782,8 @@
$links['states'][$key]
= 'ready';
}
}
+
+ $isRaw |= $module->isRaw();
}
// If there are no non-empty modules, skip this
group
@@ -2845,6 +2850,17 @@
);
} else {
$link = Html::linkedScript(
$url );
+ if ( $context->getOnly() ===
'scripts' && !$context->getRaw() && !$isRaw ) {
+ // Wrap only=script
requests in a conditional as browsers not supported
+ // by the startup
module would unconditionally execute this module.
+ // Otherwise users will
get "ReferenceError: mw is undefined" or
+ // "jQuery is
undefined" from e.g. a "site" module.
+ $link =
Html::inlineScript(
+
ResourceLoader::makeLoaderConditionalScript(
+
Xml::encodeJsCall( 'document.write', array( $link ) )
+ )
+ );
+ }
// For modules requested
directly in the html via <link> or <script>,
// tell mw.loader they are
being loading to prevent duplicate requests.
diff --git a/includes/resourceloader/ResourceLoader.php
b/includes/resourceloader/ResourceLoader.php
index 918f8be..f6a9d84 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -1002,15 +1002,6 @@
if ( count( $states ) ) {
$out .= self::makeLoaderStateScript( $states );
}
-
- if ( $context->getOnly() === 'scripts' ) {
- // In only=script requests for modules that are
not raw (e.g. not the startup module)
- // ensure the execution is conditional to avoid
situations where browsers with an
- // unsupported environment do unconditionally
execute a module's scripts. Otherwise users
- // will get things like "ReferenceError: mw is
undefined" or "jQuery is undefined" from
- // legacy scripts loaded with only=scripts
(such as the 'site' module).
- $out = self::makeLoaderConditionalScript( $out
);
- }
} else {
if ( count( $states ) ) {
$exceptions .= self::makeComment(
diff --git a/tests/phpunit/ResourceLoaderTestCase.php
b/tests/phpunit/ResourceLoaderTestCase.php
index dc5549b..f5f302e 100644
--- a/tests/phpunit/ResourceLoaderTestCase.php
+++ b/tests/phpunit/ResourceLoaderTestCase.php
@@ -46,6 +46,7 @@
protected $script = '';
protected $styles = '';
protected $skipFunction = null;
+ protected $isRaw = false;
protected $targets = array( 'test' );
public function __construct( $options = array() ) {
@@ -77,6 +78,10 @@
public function getSkipFunction() {
return $this->skipFunction;
}
+
+ public function isRaw() {
+ return $this->isRaw;
+ }
}
class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule {
diff --git a/tests/phpunit/includes/OutputPageTest.php
b/tests/phpunit/includes/OutputPageTest.php
index 97fa85c..d7e8cd3 100644
--- a/tests/phpunit/includes/OutputPageTest.php
+++ b/tests/phpunit/includes/OutputPageTest.php
@@ -141,7 +141,15 @@
// Load module script only
array(
array( 'test.foo',
ResourceLoaderModule::TYPE_SCRIPTS ),
- '<script
src="http://127.0.0.1:8080/w/load.php?debug=false&lang=en&modules=test.foo&only=scripts&skin=fallback&*";></script>
+ '<script>if(window.mw){
+document.write("\u003Cscript
src=\"http://127.0.0.1:8080/w/load.php?debug=false\u0026amp;lang=en\u0026amp;modules=test.foo\u0026amp;only=scripts\u0026amp;skin=fallback\u0026amp;*\"\u003E\u003C/script\u003E";);
+}</script>
+'
+ ),
+ array(
+ // Don't condition wrap raw modules (like the
startup module)
+ array( 'test.raw',
ResourceLoaderModule::TYPE_SCRIPTS ),
+ '<script
src="http://127.0.0.1:8080/w/load.php?debug=false&lang=en&modules=test.raw&only=scripts&skin=fallback&*";></script>
'
),
// Load module styles only
@@ -152,14 +160,10 @@
'
),
// Load private module (only=scripts)
- // This is asserted for completion (would get two
condition wrappers),
- // though in practice we'd never embed a module with
only=scripts,
- // that mode is reserved for hardcoded requests.
Embedded modules
- // would always be combined.
array(
array( 'test.quux',
ResourceLoaderModule::TYPE_SCRIPTS ),
'<script>if(window.mw){
-if(window.mw){mw.test.baz({token:123});mw.loader.state({"test.quux":"ready"});}
+mw.test.baz({token:123});mw.loader.state({"test.quux":"ready"});
}</script>
'
@@ -244,17 +248,21 @@
'styles' => '/* pref-animate=off */ .mw-icon {
transition: none; }',
'group' => 'private',
)),
+ 'test.raw' => new ResourceLoaderTestModule( array(
+ 'script' => 'mw.test.baz( { token: 123 } );',
+ 'isRaw' => true,
+ )),
'test.noscript' => new ResourceLoaderTestModule( array(
'styles' => '.mw-test-noscript { content:
"style"; }',
'group' => 'noscript',
)),
'test.group.bar' => new ResourceLoaderTestModule( array(
- 'styles' => '.mw-group-bar { content:
"style"; }',
- 'group' => 'bar',
+ 'styles' => '.mw-group-bar { content: "style";
}',
+ 'group' => 'bar',
)),
'test.group.foo' => new ResourceLoaderTestModule( array(
- 'styles' => '.mw-group-foo { content:
"style"; }',
- 'group' => 'foo',
+ 'styles' => '.mw-group-foo { content: "style";
}',
+ 'group' => 'foo',
)),
) );
$links = $method->invokeArgs( $out, $args );
--
To view, visit https://gerrit.wikimedia.org/r/159086
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iedda0464f734ba5f7a884726487f6c7e07d444f1
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
