resourceloader: Tidy up RL to simplify ResourceLoaderEditToolbarModule

* Remove ResourceLoaderFileModule::getLessCompiler(). There is no reason for a
  module to need to get a compiler in a different manner than
  ResourceLoader::getLessCompiler().
* Add ResourceLoaderModule::getLessVars(). This method provides a means for
  subclasses to easily inject custom LESS variables. The default implementation
  simply returns an empty array.
* Make the $context parameter for ResourceLoaderFileModule::readStyleFiles()
  non-optional (via graceful deprecation). The only callers I found were either
  already calling it with a ResourceLoader context, or had a perfectly usable
  ResourceLoaderContext in local scope.
* Make ResourceLoaderFileModule::{readStyleFile,getLessCompiler} require a
  context. These methods are protected, so I didn't bother with a deprecation.
* Call ksort() on the LESS variables array in the only place it matters -- when
  hashing its serialized representation to construct a cache lookup key. This
  relieves getLessVars() subclasses from having to remember to re-sort the
  variables array if they modify it.
* These changes make it possible to substantially simplify
  ResourceLoaderEditToolbarModule, because the only thing it needs to do now is
  implement its own getLessVars() method.
* This also allows it to be versioned like any other ResourceLoaderFileModule,
  rather than having to use enableModuleContentVersion().

Change-Id: Ic3eab71691e502bfe19bdf4eb6f82cc679a7782f

RELEASE-NOTES-1.26

@@ -222,6 +222,10 @@ changes to languages because of Phabricator reports.
   by &nbsp; and HTML entity encodings of &nbsp, <, and >.
 * DatabaseBase::resultObject() is now protected (use outside Database classes
   not necessary since 1.11).
+* Calling ResourceLoaderFileModule::readStyleFiles() without a
+  ResourceLoaderContext instance is deprecated.
+* ResourceLoader::getLessCompiler() now takes an optional parameter of
+  additional LESS variables to set for the compiler.
 
 == Compatibility ==
 

includes/installer/WebInstallerOutput.php

@@ -170,7 +170,8 @@ class WebInstallerOutput {
 			$styles = array_merge( $styles, ResourceLoader::makeCombinedStyles(
 				$module->readStyleFiles(
 					$module->getStyleFiles( $rlContext ),
-					$module->getFlip( $rlContext )
+					$module->getFlip( $rlContext ),
+					$rlContext
 			) ) );
 		}
 

includes/resourceloader/ResourceLoader.php

@@ -1605,12 +1605,15 @@ MESSAGE;
 	/**
 	 * Returns LESS compiler set up for use with MediaWiki
 	 *
-	 * @param Config $config
-	 * @throws MWException
 	 * @since 1.22
+	 * @since 1.26 added $extraVars parameter
+	 * @param Config $config
+	 * @param array $extraVars Associative array of extra (i.e., other than the
+	 *   globally-configured ones) that should be used for compilation.
+	 * @throws MWException
 	 * @return Less_Parser
 	 */
-	public static function getLessCompiler( Config $config ) {
+	public static function getLessCompiler( Config $config, $extraVars = array() ) {
 		// When called from the installer, it is possible that a required PHP extension
 		// is missing (at least for now; see bug 47564). If this is the case, throw an
 		// exception (caught by the installer) to prevent a fatal error later on.
@@ -1619,7 +1622,7 @@ MESSAGE;
 		}
 
 		$parser = new Less_Parser;
-		$parser->ModifyVars( self::getLessVars( $config ) );
+		$parser->ModifyVars( array_merge( self::getLessVars( $config ), $extraVars ) );
 		$parser->SetImportDirs( array_fill_keys( $config->get( 'ResourceLoaderLESSImportPaths' ), '' ) );
 		$parser->SetOption( 'relativeUrls', false );
 		$parser->SetCacheDir( $config->get( 'CacheDirectory' ) ?: wfTempDir() );
@@ -1638,8 +1641,6 @@ MESSAGE;
 		if ( !self::$lessVars ) {
 			$lessVars = $config->get( 'ResourceLoaderLESSVars' );
 			Hooks::run( 'ResourceLoaderGetLessVars', array( &$lessVars ) );
-			// Sort by key to ensure consistent hashing for cache lookups.
-			ksort( $lessVars );
 			self::$lessVars = $lessVars;
 		}
 		return self::$lessVars;

includes/resourceloader/ResourceLoaderEditToolbarModule.php

@@ -26,45 +26,19 @@
  * @since 1.24
  */
 class ResourceLoaderEditToolbarModule extends ResourceLoaderFileModule {
-
 	/**
 	 * Get language-specific LESS variables for this module.
 	 *
+	 * @since 1.26
+	 * @param ResourceLoaderContext $context
 	 * @return array
 	 */
-	private function getLessVars( ResourceLoaderContext $context ) {
+	protected function getLessVars( ResourceLoaderContext $context ) {
+		$vars = parent::getLessVars( $context );
 		$language = Language::factory( $context->getLanguage() );
-
-		// This is very conveniently formatted and we can pass it right through
-		$vars = $language->getImageFiles();
-
-		// less.php tries to be helpful and parse our variables as LESS source code
-		foreach ( $vars as $key => &$value ) {
-			$value = CSSMin::serializeStringValue( $value );
+		foreach ( $language->getImageFiles() as $key => $value ) {
+			$vars[ $key ] = CSSMin::serializeStringValue( $value );
 		}
-
 		return $vars;
 	}
-
-	/**
-	 * @return bool
-	 */
-	public function enableModuleContentVersion() {
-		return true;
-	}
-
-	/**
-	 * Get a LESS compiler instance for this module.
-	 *
-	 * Set our variables in it.
-	 *
-	 * @throws MWException
-	 * @param ResourceLoaderContext $context
-	 * @return Less_Parser
-	 */
-	protected function getLessCompiler( ResourceLoaderContext $context = null ) {
-		$parser = parent::getLessCompiler();
-		$parser->ModifyVars( $this->getLessVars( $context ) );
-		return $parser;
-	}
 }

includes/resourceloader/ResourceLoaderFileModule.php

@@ -854,13 +854,21 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 	 * @param array $styles List of media type/list of file paths pairs, to read, remap and
 	 * concetenate
 	 * @param bool $flip
-	 * @param ResourceLoaderContext $context (optional)
+	 * @param ResourceLoaderContext $context
 	 *
 	 * @throws MWException
 	 * @return array List of concatenated and remapped CSS data from $styles,
 	 *     keyed by media type
+	 *
+	 * @since 1.26 Calling this method without a ResourceLoaderContext instance
+	 *   is deprecated.
 	 */
 	public function readStyleFiles( array $styles, $flip, $context = null ) {
+		if ( $context === null ) {
+			wfDeprecated( __METHOD__ . ' without a ResourceLoader context', '1.26' );
+			$context = ResourceLoaderContext::newDummyContext();
+		}
+
 		if ( empty( $styles ) ) {
 			return array();
 		}
@@ -882,12 +890,12 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 	 *
 	 * @param string $path File path of style file to read
 	 * @param bool $flip
-	 * @param ResourceLoaderContext $context (optional)
+	 * @param ResourceLoaderContext $context
 	 *
 	 * @return string CSS data in script file
 	 * @throws MWException If the file doesn't exist
 	 */
-	protected function readStyleFile( $path, $flip, $context = null ) {
+	protected function readStyleFile( $path, $flip, $context ) {
 		$localPath = $this->getLocalPath( $path );
 		$remotePath = $this->getRemotePath( $path );
 		if ( !file_exists( $localPath ) ) {
@@ -897,8 +905,7 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 		}
 
 		if ( $this->getStyleSheetLang( $localPath ) === 'less' ) {
-			$compiler = $this->getLessCompiler( $context );
-			$style = $this->compileLessFile( $localPath, $compiler );
+			$style = $this->compileLessFile( $localPath, $context );
 			$this->hasGeneratedStyles = true;
 		} else {
 			$style = file_get_contents( $localPath );
@@ -947,12 +954,13 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 	 * Keeps track of all used files and adds them to localFileRefs.
 	 *
 	 * @since 1.22
+	 * @since 1.26 Added $context paramter.
 	 * @throws Exception If less.php encounters a parse error
 	 * @param string $fileName File path of LESS source
-	 * @param Less_Parser $parser Compiler to use, if not default
+	 * @param ResourceLoaderContext $context Context in which to generate script
 	 * @return string CSS source
 	 */
-	protected function compileLessFile( $fileName, $compiler = null ) {
+	protected function compileLessFile( $fileName, ResourceLoaderContext $context ) {
 		static $cache;
 
 		if ( !$cache ) {
@@ -961,7 +969,9 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 
 		// Construct a cache key from the LESS file name and a hash digest
 		// of the LESS variables used for compilation.
-		$varsHash = hash( 'md4', serialize( ResourceLoader::getLessVars( $this->getConfig() ) ) );
+		$vars = $this->getLessVars( $context );
+		ksort( $vars );
+		$varsHash = hash( 'md4', serialize( $vars ) );
 		$cacheKey = wfGlobalCacheKey( 'LESS', $fileName, $varsHash );
 		$cachedCompile = $cache->get( $cacheKey );
 
@@ -976,10 +986,7 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 			}
 		}
 
-		if ( !$compiler ) {
-			$compiler = $this->getLessCompiler();
-		}
-
+		$compiler = ResourceLoader::getLessCompiler( $this->getConfig(), $vars );
 		$css = $compiler->parseFile( $fileName )->getCss();
 		$files = $compiler->AllParsedFiles();
 		$this->localFileRefs = array_merge( $this->localFileRefs, $files );
@@ -993,20 +1000,6 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 		return $css;
 	}
 
-	/**
-	 * Get a LESS compiler instance for this module in given context.
-	 *
-	 * Just calls ResourceLoader::getLessCompiler() by default to get a global compiler.
-	 *
-	 * @param ResourceLoaderContext $context
-	 * @throws MWException
-	 * @since 1.24
-	 * @return Less_Parser
-	 */
-	protected function getLessCompiler( ResourceLoaderContext $context = null ) {
-		return ResourceLoader::getLessCompiler( $this->getConfig() );
-	}
-
 	/**
 	 * Takes named templates by the module and returns an array mapping.
 	 * @return array of templates mapping template alias to content

includes/resourceloader/ResourceLoaderModule.php

@@ -485,6 +485,17 @@ abstract class ResourceLoaderModule {
 		$this->msgBlobMtime[$lang] = $mtime;
 	}
 
+	/**
+	 * Get module-specific LESS variables, if any.
+	 *
+	 * @since 1.26
+	 * @param ResourceLoaderContext $context
+	 * @return array Module-specific LESS variables.
+	 */
+	protected function getLessVars( ResourceLoaderContext $context ) {
+		return array();
+	}
+
 	/**
 	 * Get an array of this module's resources. Ready for serving to the web.
 	 *

tests/phpunit/LessFileCompilationTest.php

@@ -41,11 +41,9 @@ class LessFileCompilationTest extends ResourceLoaderTestCase {
 		$rlContext = $this->getResourceLoaderContext();
 
 		// Bleh
-		$method = new ReflectionMethod( $this->module, 'getLessCompiler' );
+		$method = new ReflectionMethod( $this->module, 'compileLessFile' );
 		$method->setAccessible( true );
-		$compiler = $method->invoke( $this->module, $rlContext );
-
-		$this->assertNotNull( $compiler->parseFile( $this->file )->getCss() );
+		$this->assertNotNull( $method->invoke( $this->module, $this->file, $rlContext ) );
 	}
 
 	public function toString() {

tests/phpunit/structure/ResourcesTest.php

@@ -198,7 +198,7 @@ class ResourcesTest extends MediaWikiTestCase {
 							$media,
 							$file,
 							// XXX: Wrapped in an object to keep it out of PHPUnit output
-							(object)array( 'cssText' => $readStyleFile->invoke( $module, $file, $flip ) ),
+							(object)array( 'cssText' => $readStyleFile->invoke( $module, $file, $flip, $data['context'] ) ),
 						);
 					}
 				}