From f4f3107209866527a68ab2ad0be5ecc250fa7007 Mon Sep 17 00:00:00 2001 From: Roan Kattouw Date: Tue, 13 Sep 2011 17:13:53 +0000 Subject: [PATCH] Fix the fixme on r88053: dependency handling was broken in debug mode in certain cases. More specifically, if A is a file module that depends on B, B is a wiki module that depends on C and C is a file module, the loading order is CBA (correct) in production mode but was BCA (wrong) in debug mode. Fixed this by URL-ifying scripts and styles for those modules in debug mode, as I said to on CR. What this means is that the initial debug=true request for a module will now always return arrays of URLs, never the JS or CSS itself. This was already the case for file modules (which returned arrays of URLs to the raw files), but not for other modules (which returned the JS and CSS itself). So for non-file modules, load.php?modules=foo&debug=true now returns some JS that instructs the loader to fetch the module's JS from load.php?modules=foo&debug=true&only=scripts and the CSS from ...&only=styles . * Removed the magic behavior where ResourceLoaderModule::getScripts() and getStyles() could return an array of URLs where the documentation said they should return a JS/CSS string. Because I didn't restructure the calling code too much, the old magical behavior should still work. * Instead, move this behavior to getScriptURLsForDebug() and getStyleURLsForDebug(). The default implementation constructs a single URL for a load.php request for the module with debug=true&only=scripts (or styles). The URL building code duplicates some things from OutputPage::makeResourceLoaderLink(), I'll clean that up later. ResourceLoaderFileModule overrides this method to return URLs to the raw files, using code that I removed from getScripts()/getStyles() * Add ResourceLoaderModule::supportsURLLoading(), which returns true by default but may return false to indicate that a module does not support loading via a URL. This is needed to respect $this->debugRaw in ResourceLoaderFileModule (set to true for jquery and mediawiki), and obviously for the startup module as well, because we get bootstrapping problems otherwise (can't call mw.loader.implement() when the code for mw.loader isn't loaded yet) --- includes/resourceloader/ResourceLoader.php | 26 ++++++-- .../ResourceLoaderFileModule.php | 40 ++++++------ .../resourceloader/ResourceLoaderModule.php | 61 +++++++++++++++++++ .../ResourceLoaderStartUpModule.php | 4 ++ 4 files changed, 108 insertions(+), 23 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 57c179ba98..49111652e6 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -529,17 +529,31 @@ class ResourceLoader { try { $scripts = ''; if ( $context->shouldIncludeScripts() ) { - $scripts = $module->getScript( $context ); - if ( is_string( $scripts ) ) { - // bug 27054: Append semicolon to prevent weird bugs - // caused by files not terminating their statements right - $scripts .= ";\n"; + // If we are in debug mode, we'll want to return an array of URLs if possible + // However, we can't do this if the module doesn't support it + // We also can't do this if there is an only= parameter, because we have to give + // the module a way to return a load.php URL without causing an infinite loop + if ( $context->getDebug() && !$context->getOnly() && $module->supportsURLLoading() ) { + $scripts = $module->getScriptURLsForDebug( $context ); + } else { + $scripts = $module->getScript( $context ); + if ( is_string( $scripts ) ) { + // bug 27054: Append semicolon to prevent weird bugs + // caused by files not terminating their statements right + $scripts .= ";\n"; + } } } // Styles $styles = array(); if ( $context->shouldIncludeStyles() ) { - $styles = $module->getStyles( $context ); + // If we are in debug mode, we'll want to return an array of URLs + // See comment near shouldIncludeScripts() for more details + if ( $context->getDebug() && !$context->getOnly() && $module->supportsURLLoading() ) { + $styles = $module->getStyleURLsForDebug( $context ); + } else { + $styles = $module->getStyles( $context ); + } } // Messages diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index ca215d5e10..6b30eb192d 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -217,15 +217,20 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { */ public function getScript( ResourceLoaderContext $context ) { $files = $this->getScriptFiles( $context ); - if ( $context->getDebug() && $this->debugRaw ) { - $urls = array(); - foreach ( $this->getScriptFiles( $context ) as $file ) { - $urls[] = $this->getRemotePath( $file ); - } - return $urls; - } return $this->readScriptFiles( $files ); } + + public function getScriptURLsForDebug( ResourceLoaderContext $context ) { + $urls = array(); + foreach ( $this->getScriptFiles( $context ) as $file ) { + $urls[] = $this->getRemotePath( $file ); + } + return $urls; + } + + public function supportsURLLoading() { + return $this->debugRaw; + } /** * Gets loader script. @@ -250,16 +255,6 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { $this->getStyleFiles( $context ), $this->getFlip( $context ) ); - if ( !$context->getOnly() && $context->getDebug() && $this->debugRaw ) { - $urls = array(); - foreach ( $this->getStyleFiles( $context ) as $mediaType => $list ) { - $urls[$mediaType] = array(); - foreach ( $list as $file ) { - $urls[$mediaType][] = $this->getRemotePath( $file ); - } - } - return $urls; - } // Collect referenced files $this->localFileRefs = array_unique( $this->localFileRefs ); // If the list has been modified since last time we cached it, update the cache @@ -276,6 +271,17 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { return $styles; } + public function getStyleURLsForDebug( ResourceLoaderContext $context ) { + $urls = array(); + foreach ( $this->getStyleFiles( $context ) as $mediaType => $list ) { + $urls[$mediaType] = array(); + foreach ( $list as $file ) { + $urls[$mediaType][] = $this->getRemotePath( $file ); + } + } + return $urls; + } + /** * Gets list of message keys used by this module. * diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 49e9ef60da..fb6a503ea3 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -126,6 +126,44 @@ abstract class ResourceLoaderModule { // Stub, override expected return ''; } + + /** + * Get the URL or URLs to load for this module's JS in debug mode. + * The default behavior is to return a load.php?only=scripts URL for + * the module, but file-based modules will want to override this to + * load the files directly. + * + * This function is called only when 1) we're in debug mode, 2) there + * is no only= parameter and 3) supportsURLLoading() returns true. + * #2 is important to prevent an infinite loop, therefore this function + * MUST return either an only= URL or a non-load.php URL. + * + * @param $context ResourceLoaderContext: Context object + * @return Array of URLs + */ + public function getScriptURLsForDebug( ResourceLoaderContext $context ) { + global $wgLoadScript; // TODO factor out to ResourceLoader static method and deduplicate from makeResourceLoaderLink() + $query = array( + 'modules' => $this->getName(), + 'only' => 'scripts', + 'skin' => $context->getSkin(), + 'user' => $context->getUser(), + 'debug' => 'true', + 'version' => $context->getVersion() + ); + ksort( $query ); + return array( wfAppendQuery( $wgLoadScript, $query ) . '&*' ); + } + + /** + * Whether this module supports URL loading. If this function returns false, + * getScript() will be used even in cases (debug mode, no only param) where + * getScriptURLsForDebug() would normally be used instead. + * @return bool + */ + public function supportsURLLoading() { + return true; + } /** * Get all CSS for this module for a given skin. @@ -137,6 +175,29 @@ abstract class ResourceLoaderModule { // Stub, override expected return array(); } + + /** + * Get the URL or URLs to load for this module's CSS in debug mode. + * The default behavior is to return a load.php?only=styles URL for + * the module, but file-based modules will want to override this to + * load the files directly. See also getScriptURLsForDebug() + * + * @param $context ResourceLoaderContext: Context object + * @return Array: array( mediaType => array( URL1, URL2, ... ), ... ) + */ + public function getStyleURLsForDebug( ResourceLoaderContext $context ) { + global $wgLoadScript; // TODO factor out to ResourceLoader static method and deduplicate from makeResourceLoaderLink() + $query = array( + 'modules' => $this->getName(), + 'only' => 'styles', + 'skin' => $context->getSkin(), + 'user' => $context->getUser(), + 'debug' => 'true', + 'version' => $context->getVersion() + ); + ksort( $query ); + return array( 'all' => array( wfAppendQuery( $wgLoadScript, $query ) . '&*' ) ); + } /** * Get the messages needed for this module. diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php index 40786fa732..7b819def06 100644 --- a/includes/resourceloader/ResourceLoaderStartUpModule.php +++ b/includes/resourceloader/ResourceLoaderStartUpModule.php @@ -241,6 +241,10 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { return $out; } + public function supportsURLLoading() { + return false; + } + /** * @param $context ResourceLoaderContext * @return array|mixed -- 2.20.1