r72797 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r72796‎ | r72797 | r72798 >
Date:10:20, 11 September 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Made CSSMin::remap take both local and remote directory parameters, allowing more felxible configuration. Also made ResourceLoaderFileModule use $wgScriptPath as the base of the remote directory parameter.
Modified paths:
  • /trunk/phase3/includes/ResourceLoaderModule.php (modified) (history)
  • /trunk/phase3/includes/libs/CSSMin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/libs/CSSMin.php
@@ -18,7 +18,7 @@
1919 * This class provides minification, URL remapping, URL extracting, and data-URL embedding.
2020 *
2121 * @file
22 - * @version 0.1.0 -- 2010-09-09
 22+ * @version 0.1.1 -- 2010-09-11
2323 * @author Trevor Parscal <tparscal@wikimedia.org>
2424 * @copyright Copyright 2010 Wikimedia Foundation
2525 * @license http://www.apache.org/licenses/LICENSE-2.0
@@ -81,7 +81,7 @@
8282 * @param $path string File path where the source was read from
8383 * @return string Remapped CSS data
8484 */
85 - public static function remap( $source, $path, $embed = true ) {
 85+ public static function remap( $source, $local, $remote, $embed = true ) {
8686 $pattern = '/((?<embed>\s*\/\*\s*\@embed\s*\*\/)(?<pre>[^\;\}]*))?' . self::URL_REGEX . '(?<post>[^;]*)[\;]?/';
8787 $offset = 0;
8888 while ( preg_match( $pattern, $source, $match, PREG_OFFSET_CAPTURE, $offset ) ) {
@@ -89,11 +89,12 @@
9090 $embed = $match['embed'][0];
9191 $pre = $match['pre'][0];
9292 $post = $match['post'][0];
93 - $file = "{$path}/{$match['file'][0]}";
94 - // Only proceed if we can access the file
 93+ $file = "{$local}/{$match['file'][0]}";
 94+ $url = "{$remote}/{$match['file'][0]}";
 95+ // Only proceed if we can access the fill
9596 if ( file_exists( $file ) ) {
9697 // Add version parameter as a time-stamp in ISO 8601 format, using Z for the timezone, meaning GMT
97 - $url = "{$file}?" . gmdate( 'Y-m-d\TH:i:s\Z', round( filemtime( $file ), -2 ) );
 98+ $url .= '?' . gmdate( 'Y-m-d\TH:i:s\Z', round( filemtime( $file ), -2 ) );
9899 // If we the mime-type can't be determined, no embedding will take place
99100 $type = false;
100101 // Try a couple of different ways to get the mime-type of a file, in order of preference
Index: trunk/phase3/includes/ResourceLoaderModule.php
@@ -684,8 +684,13 @@
685685 * @return string Remapped CSS
686686 */
687687 protected static function remapStyle( $file ) {
688 - global $wgUseDataURLs;
689 - return CSSMin::remap( file_get_contents( self::remapFilename( $file ) ), dirname( $file ), $wgUseDataURLs );
 688+ global $wgUseDataURLs, $wgScriptPath;
 689+ return CSSMin::remap(
 690+ file_get_contents( self::remapFilename( $file ) ),
 691+ dirname( $file ),
 692+ $wgScriptPath . '/' . dirname( $file ),
 693+ $wgUseDataURLs
 694+ );
690695 }
691696 }
692697

Comments

#Comment by Catrope (talk | contribs)   16:10, 27 September 2010
+			// Only proceed if we can access the fill

That might be a problem for servers located in, say, mountainous areas ;)

Seriously, though, this revision doesn't add the flexibility that I think really matters and was previously supported: allowing the skins/ and extensions/ directory to be hosted at different locations. To achieve this, modules should be able to specify the URL and FS paths to their resources separately (otherwise the current default will be used) and use $wgStylePath or $wgExtensionAssetsPath or dirname( __FILE__ ) or whatever they deem appropriate.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:43, 27 September 2010

How would to get file mtime information from remote hosted files? r73819 fixes the typo.

#Comment by Catrope (talk | contribs)   18:48, 27 September 2010

You wouldn't. You'd be able to either:

  1. only specify a local path. Remote path extrapolation with $wgScriptPath works the way it does right now
  2. specify a remote path that's different from the extraploated one mentioned above
  3. only specify a remote path, in which case the resource loader is limited in what it can do
#Comment by Trevor Parscal (WMF) (talk | contribs)   18:53, 27 September 2010

This revision was actually focused on moving a MediaWiki global dependency out of a stand-alone library. It doesn't actually change the kind of behavior you are talking about. Enhancement bug?

#Comment by Catrope (talk | contribs)   18:57, 27 September 2010

Oh, I didn't get that that was the intention. Enhancement bug, yes.

Status & tagging log