r94421 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r94420‎ | r94421 | r94422 >
Date:17:27, 13 August 2011
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
(bug 26283) Previewing user JS/CSS pages doesn't load other user JS/CSS pages
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderUserModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
@@ -44,6 +44,7 @@
4545 * (bug 30264) Changed installer-generated LocalSettings.php to use require_once()
4646 instead require() for included extensions.
4747 * Do not convert text in the user interface language to another script.
 48+* (bug 26283) Previewing user JS/CSS pages doesn't load other user JS/CSS pages
4849
4950 === API changes in 1.19 ===
5051 * (bug 19838) siprop=interwikimap can now use the interwiki cache.
Index: trunk/phase3/includes/resourceloader/ResourceLoaderUserModule.php
@@ -35,7 +35,7 @@
3636 protected function getPages( ResourceLoaderContext $context ) {
3737 if ( $context->getUser() ) {
3838 $username = $context->getUser();
39 - return array(
 39+ $pages = array(
4040 "User:$username/common.js" => array( 'type' => 'script' ),
4141 "User:$username/" . $context->getSkin() . '.js' =>
4242 array( 'type' => 'script' ),
@@ -43,6 +43,15 @@
4444 "User:$username/" . $context->getSkin() . '.css' =>
4545 array( 'type' => 'style' ),
4646 );
 47+
 48+ // Hack for bug 26283: if we're on a preview page for a CSS/JS page,
 49+ // we need to exclude that page from this module. In that case, the excludepage
 50+ // parameter will be set to the name of the page we need to exclude.
 51+ $excludepage = $context->getRequest()->getVal( 'excludepage' );
 52+ if ( isset( $pages[$excludepage] ) ) {
 53+ unset( $pages[$excludepage] );
 54+ }
 55+ return $pages;
4756 }
4857 return array();
4958 }
Index: trunk/phase3/includes/OutputPage.php
@@ -2347,19 +2347,20 @@
23482348
23492349 /**
23502350 * TODO: Document
2351 - * @param $modules Array/string with the module name
 2351+ * @param $modules Array/string with the module name(s)
23522352 * @param $only String ResourceLoaderModule TYPE_ class constant
23532353 * @param $useESI boolean
 2354+ * @param $extraQuery Array with extra query parameters to add to each request. array( param => value )
23542355 * @return string html <script> and <style> tags
23552356 */
2356 - protected function makeResourceLoaderLink( $modules, $only, $useESI = false ) {
 2357+ protected function makeResourceLoaderLink( $modules, $only, $useESI = false, array $extraQuery = array() ) {
23572358 global $wgLoadScript, $wgResourceLoaderUseESI,
23582359 $wgResourceLoaderInlinePrivateModules;
23592360 $baseQuery = array(
23602361 'lang' => $this->getContext()->getLang()->getCode(),
23612362 'debug' => ResourceLoader::inDebugMode() ? 'true' : 'false',
23622363 'skin' => $this->getSkin()->getSkinName(),
2363 - );
 2364+ ) + $extraQuery;
23642365 if ( $only !== ResourceLoaderModule::TYPE_COMBINED ) {
23652366 $baseQuery['only'] = $only;
23662367 }
@@ -2577,11 +2578,15 @@
25782579 if ( $wgAllowUserJs && $this->getUser()->isLoggedIn() ) {
25792580 if( $this->getTitle() && $this->getTitle()->isJsSubpage() && $this->userCanPreview() ) {
25802581 # XXX: additional security check/prompt?
 2582+ // We're on a preview of a JS subpage
 2583+ // Exclude this page from the user module in case it's in there (bug 26283)
 2584+ $scripts .= $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS, false,
 2585+ array( 'excludepage' => $this->getTitle()->getPrefixedText() )
 2586+ );
 2587+ // Load the previewed JS
25812588 $scripts .= Html::inlineScript( "\n" . $this->getRequest()->getText( 'wpTextbox1' ) . "\n" ) . "\n";
25822589 } else {
2583 - # @todo FIXME: This means that User:Me/Common.js doesn't load when previewing
2584 - # User:Me/Vector.js, and vice versa (bug 26283)
2585 -
 2590+ // Include the user module normally
25862591 // We can't do $userScripts[] = 'user'; because the user module would end up
25872592 // being wrapped in a closure, so load it raw like 'site'
25882593 $scripts .= $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_SCRIPTS );
@@ -2961,6 +2966,7 @@
29622967 // Add ResourceLoader styles
29632968 // Split the styles into four groups
29642969 $styles = array( 'other' => array(), 'user' => array(), 'site' => array(), 'private' => array(), 'noscript' => array() );
 2970+ $otherTags = ''; // Tags to append after the normal <link> tags
29652971 $resourceLoader = $this->getResourceLoader();
29662972
29672973 $moduleStyles = $this->getModuleStyles();
@@ -2977,9 +2983,15 @@
29782984 // Per-user custom styles
29792985 if ( $wgAllowUserCss ) {
29802986 if ( $this->getTitle()->isCssSubpage() && $this->userCanPreview() ) {
2981 - // @todo FIXME: Properly escape the cdata!
2982 - $this->addInlineStyle( $this->getRequest()->getText( 'wpTextbox1' ) );
 2987+ // We're on a preview of a CSS subpage
 2988+ // Exclude this page from the user module in case it's in there (bug 26283)
 2989+ $otherTags .= $this->makeResourceLoaderLink( 'user', ResourceLoaderModule::TYPE_STYLES, false,
 2990+ array( 'excludepage' => $this->getTitle()->getPrefixedText() )
 2991+ );
 2992+ // Load the previewed CSS
 2993+ $otherTags .= Html::inlineStyle( $this->getRequest()->getText( 'wpTextbox1' ) );;
29832994 } else {
 2995+ // Load the user styles normally
29842996 $moduleStyles[] = 'user';
29852997 }
29862998 }
@@ -3015,6 +3027,9 @@
30163028 ResourceLoaderModule::TYPE_STYLES
30173029 );
30183030 }
 3031+
 3032+ // Add stuff in $otherTags (previewed user CSS if applicable)
 3033+ $ret .= $otherTags;
30193034 return $ret;
30203035 }
30213036

Follow-up revisions

RevisionCommit summaryAuthorDate
r94766Followup r94421 which, per CR, broke in non-English content languages because...catrope14:55, 17 August 2011
r107066apply CSS::Janus when user preview its CSS style...hashar13:35, 22 December 2011
r108078Redo r107066 properly: apply CSSJanus to user CSS previews when needed. Ping ...catrope20:01, 4 January 2012

Comments

#Comment by Krinkle (talk | contribs)   18:45, 14 August 2011
array( 'excludepage' => $this->getTitle()->getPrefixedText() )
...
$username = $context->getUser();
...
"User:$username/" . $context->getSkin() . '.css'
...
$excludepage = $context->getRequest()->getVal( 'excludepage' );
			if ( isset( $pages[$excludepage] ) ) {
				unset( $pages[$excludepage] );

Does getPrefixedText() use the same format as getUser() (ie. encoding and underscores) ?

+				$otherTags .= Html::inlineStyle( $this->getRequest()->getText( 'wpTextbox1' ) );;

Bonus semi-colon

#Comment by Krinkle (talk | contribs)   18:46, 14 August 2011

also namespace name may be localized as supposed to hardcoded English. Perhaps a unit test (if possible).

#Comment by Catrope (talk | contribs)   21:08, 14 August 2011

All good points. This will almost certainly fail with localized namespace names, and may fail due to underscores. Will test for these things and fix any issues.

#Comment by Catrope (talk | contribs)   14:56, 17 August 2011

Fixed in r94766. Localized namespace names were indeed causing problems. Spaces weren't, but that was only by accident (I was using two different functions that both happened to output spaces). Now using getPrefixedDBkey() so things are always converted to the native namespace name with underscores before comparing them.

#Comment by Schnark (talk | contribs)   07:38, 29 August 2011

I didn't test it, but I think this change will cause undesired behavior when previewing JS/CSS-subpages that are imported by common.js/...

Let's assume Special:MyPage/common.js contains

importScript('User:USERNAME/script.js');

If now I preview changes made to Special:MyPage/script.js this script will get executed twice: The old version via common.js and importScript, the new version via the inline script. This is likely to cause some trouble.

#Comment by Catrope (talk | contribs)   07:58, 29 August 2011

This problem isn't caused by this revision: it existed before too.

#Comment by Catrope (talk | contribs)   07:59, 29 August 2011

...or well, I guess it didn't, because we didn't load any other JS when you previewed a JS page.

Yes, it isn't perfect and will break in some cases. The old behavior was less perfect and broke in more cases, so this is an improvement.

Also, in The Future (TM) you shouldn't be importScript()ing pages from common.js any more (it causes extra requests that don't go through ResourceLoader) but use something module-based instead.

#Comment by Hashar (talk | contribs)   13:49, 6 December 2011

Krinkle / Catrope, is there anything else to add on this revision or can it be considered safe now? Last update was at the end of August.

#Comment by Hashar (talk | contribs)   14:18, 21 December 2011

When loading the user CSS content you replaced $this->addInlineStyle() by Html::inlineStyle()

The former pass the content through CSS::janus(), the later does not. That might cause troubles when previewing CSS that use left/right since the actual rendering will be different on RTL wikis.

Might be an issue with Html::inlineStyle() which should pass content though CSS::janus().

#Comment by Catrope (talk | contribs)   14:21, 21 December 2011

We're dealing with site/user JS, though, which is only Janused if the user language directionality differs from the site language directionality (in other words, on RTL wikis .js pages are assumed to be RTL and are only flipped if the user language is LTR). So it's probably a rare case, but you make a good point.

Status & tagging log