r81692 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r81691‎ | r81692 | r81693 >
Date:06:34, 8 February 2011
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Fixes for ResourceLoaderWikiModule r72776. No serious bugs found, do not merge before deployment.
* Specify page titles as strings instead of split NS/DBK, as suggested by Roan on CR. It seemed sensible to me.
* Pass a Title object to getContent() instead of a string, to avoid unnecessary object construction overhead
* "*" and "/" are valid title characters. Check module input for JS comment end tokens.
* Fixed inappropriate conversion to boolean, when checking result of getContent(). Presumably the idea was to omit empty sections and errors, so that's what I did. Maybe an informative error message would be better in the error case.
* Use LinkBatch for selecting multiple page rows instead of Database::makeWhereFrom2d().
* Fixed assignment expression.
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoaderSiteModule.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderUserModule.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderWikiModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoaderSiteModule.php
——@@ -36,15 +36,14 @@
3737 global $wgHandheldStyle;
3838
3939 $pages = array(
40 - 'Common.js' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'script' ),
41 - 'Common.css' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'style' ),
42 - ucfirst( $context->getSkin() ) . '.js' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'script' ),
43 - ucfirst( $context->getSkin() ) . '.css' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'style' ),
44 - 'Print.css' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'style', 'media' => 'print' ),
 40+ 'MediaWiki:Common.js' => array( 'type' => 'script' ),
 41+ 'MediaWiki:Common.css' => array( 'type' => 'style' ),
 42+ 'MediaWiki:' . ucfirst( $context->getSkin() ) . '.js' => array( 'type' => 'script' ),
 43+ 'MediaWiki:' . ucfirst( $context->getSkin() ) . '.css' => array( 'type' => 'style' ),
 44+ 'MediaWiki:Print.css' => array( 'type' => 'style', 'media' => 'print' ),
4545 );
4646 if ( $wgHandheldStyle ) {
47 - $pages['Handheld.css'] = array(
48 - 'ns' => NS_MEDIAWIKI,
 47+ $pages['MediaWiki:Handheld.css'] = array(
4948 'type' => 'style',
5049 'media' => 'handheld' );
5150 }
Index: trunk/phase3/includes/resourceloader/ResourceLoaderUserModule.php
——@@ -32,12 +32,12 @@
3333 if ( $context->getUser() ) {
3434 $username = $context->getUser();
3535 return array(
36 - "$username/common.js" => array( 'ns' => NS_USER, 'type' => 'script' ),
37 - "$username/" . $context->getSkin() . '.js' =>
38 - array( 'ns' => NS_USER, 'type' => 'script' ),
39 - "$username/common.css" => array( 'ns' => NS_USER, 'type' => 'style' ),
40 - "$username/" . $context->getSkin() . '.css' =>
41 - array( 'ns' => NS_USER, 'type' => 'style' ),
 36+ "User:$username/common.js" => array( 'type' => 'script' ),
 37+ "User:$username/" . $context->getSkin() . '.js' =>
 38+ array( 'type' => 'script' ),
 39+ "User:$username/common.css" => array( 'type' => 'style' ),
 40+ "User:$username/" . $context->getSkin() . '.css' =>
 41+ array( 'type' => 'style' ),
4242 );
4343 }
4444 return array();
Index: trunk/phase3/includes/resourceloader/ResourceLoaderWikiModule.php
——@@ -45,12 +45,12 @@
4646
4747 /* Protected Methods */
4848
49 - protected function getContent( $page, $ns ) {
50 - if ( $ns === NS_MEDIAWIKI ) {
51 - return wfEmptyMsg( $page ) ? '' : wfMsgExt( $page, 'content' );
 49+ protected function getContent( $title ) {
 50+ if ( $title->getNamespace() === NS_MEDIAWIKI ) {
 51+ $dbkey = $title->getDBkey();
 52+ return wfEmptyMsg( $dbkey ) ? '' : wfMsgExt( $dbkey, 'content' );
5253 }
53 - $title = Title::newFromText( $page, $ns );
54 - if ( !$title || !$title->isCssJsSubpage() ) {
 54+ if ( !$title->isCssJsSubpage() ) {
5555 return null;
5656 }
5757 $revision = Revision::newFromTitle( $title );
——@@ -64,15 +64,21 @@
6565
6666 public function getScript( ResourceLoaderContext $context ) {
6767 $scripts = '';
68 - foreach ( $this->getPages( $context ) as $page => $options ) {
 68+ foreach ( $this->getPages( $context ) as $titleText => $options ) {
6969 if ( $options['type'] !== 'script' ) {
7070 continue;
7171 }
72 - $script = $this->getContent( $page, $options['ns'] );
73 - if ( $script ) {
74 - $ns = MWNamespace::getCanonicalName( $options['ns'] );
75 - $scripts .= "/* $ns:$page */\n$script\n";
 72+ $title = Title::newFromText( $titleText );
 73+ if ( !$title ) {
 74+ continue;
7675 }
 76+ $script = $this->getContent( $title );
 77+ if ( strval( $script ) !== '' ) {
 78+ if ( strpos( $titleText, '*/' ) === false ) {
 79+ $scripts .= "/* $titleText */\n";
 80+ }
 81+ $scripts .= $script . "\n";
 82+ }
7783 }
7884 return $scripts;
7985 }
——@@ -80,13 +86,17 @@
8187 public function getStyles( ResourceLoaderContext $context ) {
8288
8389 $styles = array();
84 - foreach ( $this->getPages( $context ) as $page => $options ) {
 90+ foreach ( $this->getPages( $context ) as $titleText => $options ) {
8591 if ( $options['type'] !== 'style' ) {
8692 continue;
8793 }
 94+ $title = Title::newFromText( $titleText );
 95+ if ( !$title ) {
 96+ continue;
 97+ }
8898 $media = isset( $options['media'] ) ? $options['media'] : 'all';
89 - $style = $this->getContent( $page, $options['ns'] );
90 - if ( !$style ) {
 99+ $style = $this->getContent( $title );
 100+ if ( strval( $style ) === '' ) {
91101 continue;
92102 }
93103 if ( $this->getFlip( $context ) ) {
——@@ -95,8 +105,10 @@
96106 if ( !isset( $styles[$media] ) ) {
97107 $styles[$media] = '';
98108 }
99 - $ns = MWNamespace::getCanonicalName( $options['ns'] );
100 - $styles[$media] .= "/* $ns:$page */\n$style\n";
 109+ if ( strpos( $titleText, '*/' ) === false ) {
 110+ $styles[$media] .= "/* $titleText */\n";
 111+ }
 112+ $styles[$media] .= $style . "\n";
101113 }
102114 return $styles;
103115 }
——@@ -107,17 +119,16 @@
108120 return $this->modifiedTime[$hash];
109121 }
110122
111 - $titles = array();
112 - foreach ( $this->getPages( $context ) as $page => $options ) {
113 - $titles[$options['ns']][$page] = true;
 123+ $batch = new LinkBatch;
 124+ foreach ( $this->getPages( $context ) as $titleText => $options ) {
 125+ $batch->addObj( Title::newFromText( $titleText ) );
114126 }
115127
116128 $modifiedTime = 1; // wfTimestamp() interprets 0 as "now"
117 -
118 - if ( $titles ) {
 129+ if ( !$batch->isEmpty() ) {
119130 $dbr = wfGetDB( DB_SLAVE );
120131 $latest = $dbr->selectField( 'page', 'MAX(page_touched)',
121 - $dbr->makeWhereFrom2d( $titles, 'page_namespace', 'page_title' ),
 132+ $batch->constructSet( 'page', $dbr ),
122133 __METHOD__ );
123134
124135 if ( $latest ) {
——@@ -125,6 +136,7 @@
126137 }
127138 }
128139
129 - return $this->modifiedTime[$hash] = $modifiedTime;
 140+ $this->modifiedTime[$hash] = $modifiedTime;
 141+ return $modifiedTime;
130142 }
131143 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r822871.17wmf1: Fixing timestamp bug for users with spaces in their name. Already f...catrope20:06, 16 February 2011
r82524Adapt Gadgets to changes in r81692maxsem18:14, 20 February 2011
r846131.17wmf1: MFT r81692, r82468, r83814, r83885, r83891, r83897, r83902, r83903,...catrope17:42, 23 March 2011
r85148MFT: r80495, r80610, r80765, r81177, r81490, r81692, r81707, r81729, r81765, ...demon20:11, 1 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r72776Fixed user scripts/styles and site scripts/styles - they were totally broken ...tparscal07:33, 11 September 2010

Comments

#Comment by MaxSem (talk | contribs)   06:55, 8 February 2011

As a reminder, at least Gadgets need to adapt to this change.

#Comment by MaxSem (talk | contribs)   18:15, 20 February 2011

Tagged for backporting to 1.17, we don't want to introduce RL just to break extensions in the next version.

Status & tagging log