From 3f1e9fa268cdc9850dd1df2b271dbd814d957488 Mon Sep 17 00:00:00 2001 From: Ori Livneh Date: Fri, 25 Sep 2015 10:57:35 -0700 Subject: [PATCH] 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 | 4 ++ includes/installer/WebInstallerOutput.php | 3 +- includes/resourceloader/ResourceLoader.php | 11 ++--- .../ResourceLoaderEditToolbarModule.php | 38 +++------------- .../ResourceLoaderFileModule.php | 45 ++++++++----------- .../resourceloader/ResourceLoaderModule.php | 11 +++++ tests/phpunit/LessFileCompilationTest.php | 6 +-- tests/phpunit/structure/ResourcesTest.php | 2 +- 8 files changed, 51 insertions(+), 69 deletions(-) diff --git a/RELEASE-NOTES-1.26 b/RELEASE-NOTES-1.26 index 7f738e17b0..802a963fc5 100644 --- a/RELEASE-NOTES-1.26 +++ b/RELEASE-NOTES-1.26 @@ -222,6 +222,10 @@ changes to languages because of Phabricator reports. by   and HTML entity encodings of  , <, 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 == diff --git a/includes/installer/WebInstallerOutput.php b/includes/installer/WebInstallerOutput.php index 0ccdb11af9..211bad1e0b 100644 --- a/includes/installer/WebInstallerOutput.php +++ b/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 ) ) ); } diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 4a9cd0e21b..14dd633219 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -1605,12 +1605,15 @@ MESSAGE; /** * Returns LESS compiler set up for use with MediaWiki * + * @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 - * @since 1.22 * @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; diff --git a/includes/resourceloader/ResourceLoaderEditToolbarModule.php b/includes/resourceloader/ResourceLoaderEditToolbarModule.php index ef51e0cbd3..fca7961bcf 100644 --- a/includes/resourceloader/ResourceLoaderEditToolbarModule.php +++ b/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; - } } diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index ca10ab7cc7..0df28929ce 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/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 diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 80c8220550..1551ae3b64 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/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. * diff --git a/tests/phpunit/LessFileCompilationTest.php b/tests/phpunit/LessFileCompilationTest.php index eec02edc64..5e1f1a9666 100644 --- a/tests/phpunit/LessFileCompilationTest.php +++ b/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() { diff --git a/tests/phpunit/structure/ResourcesTest.php b/tests/phpunit/structure/ResourcesTest.php index eefc926a77..23afabdfd7 100644 --- a/tests/phpunit/structure/ResourcesTest.php +++ b/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'] ) ), ); } } -- 2.20.1