r81731 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r81730‎ | r81731 | r81732 >
Date:12:34, 8 February 2011
Author:catrope
Status:ok (Comments)
Tags:
Comment:
Remove $wgServer prepending from remote JS/CSS paths. It's not needed and breaks other-domain $wgStylePath / $wgExtensionAssetsPath settings
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php
——@@ -206,8 +206,6 @@
207207 * @return String: JavaScript code for $context
208208 */
209209 public function getScript( ResourceLoaderContext $context ) {
210 - global $wgServer;
211 -
212210 $files = array_merge(
213211 $this->scripts,
214212 self::tryForKey( $this->languageScripts, $context->getLanguage() ),
——@@ -218,7 +216,7 @@
219217 if ( $this->debugRaw ) {
220218 $script = '';
221219 foreach ( $files as $file ) {
222 - $path = $wgServer . $this->getRemotePath( $file );
 220+ $path = $this->getRemotePath( $file );
223221 $script .= "\n\t" . Xml::encodeJsCall( 'mediaWiki.loader.load', array( $path ) );
224222 }
225223 return $script;

Follow-up revisions

RevisionCommit summaryAuthorDate
r817321.17wmf1: MFT r81731catrope12:35, 8 February 2011
r82038Follow-up r81731 CR comments. Calling wfExpandUrl() on remote base path.krinkle22:46, 12 February 2011
r82039Follow-up r82038. Calling wfExpandUrl() on remote base path *AFTER* is has be...krinkle23:20, 12 February 2011
r864741.17: MFT r81731, r85377, r85547, r85555, r85583, r85803, r85881, r86100, r86...catrope13:22, 20 April 2011

Comments

#Comment by Krinkle (talk | contribs)   12:38, 8 February 2011

Paths are relative by default /w/skins will point to the current domain anyway. And when they're not relative, they're complete with domain.

Making 'ok'.

#Comment by Brion VIBBER (talk | contribs)   01:45, 12 February 2011

This breaks debug mode: only full URLs starting with 'http:' or 'https:' actually get loaded.

My provisional fix on bugzilla:27321 changes mediawiki.loader.load to accept local paths starting with "/" as well; Roan & Trevor, would you prefer that change or reverting this one? Would accepting local paths otherwise be a problem?

#Comment by Brion VIBBER (talk | contribs)   02:01, 12 February 2011

Note also that wfExpandUrl() will happily prepend the $wgServer for what were local paths without breaking those that were pointing offsite already.

#Comment by Krinkle (talk | contribs)   02:05, 12 February 2011

r73093 added support for requesting http/https requests (in addition to relative (ie. starting with "index.php") or absolute paths without a domain (ie. starting with "/resources") ) as well.

However currently requesting after failing the if() check for http:// returns true but does not add a script tag. I'm guessing request() is not called in the else{} part at the bottom of mw.loader.load

#Comment by Krinkle (talk | contribs)   02:06, 12 February 2011

r73093 added support for requesting http/https requests (in addition to relative (ie. starting with "index.php") or absolute paths without a domain (ie. starting with "/resources") ) as well. However currently requesting: 

mediaWiki.loader.implement("jquery.client", function( $, mw ) {
	mediaWiki.loader.load("/w/resources/jquery/jquery.client.js");

}, {}, {});

will, after failing the if() check for http://, return true. But does not add a script tag. I'm guessing request() is not called in the else{} part at the bottom of mw.loader.load

#Comment by Fomafix (talk | contribs)   14:29, 7 April 2011

Without this patch I get in 1.17 at the moment 

mediaWiki.loader.implement("jquery.autoEllipsis", function( $, mw ) {
	mediaWiki.loader.load("http://www.example.comhttp://www.example.com/mediawiki-1.17/resources/jquery/jquery.autoEllipsis.js");
#Comment by Catrope (talk | contribs)   14:31, 7 April 2011

Marking new, this was fixed in the followup revs ages ago. Also tagging 1.17.

Status & tagging log