From: Kunal Mehta <legoktm@gmail.com>
Date: Mon, 25 Aug 2014 08:02:48 +0000 (-0700)
Subject: resourceloader: Only store sources' load.php urls
X-Git-Tag: 1.31.0-rc.0~14123^2
X-Git-Url: https://git.cyclocoop.org/%28%28?a=commitdiff_plain;h=e103ba265bbcdf9521ffbfdfde7ea9c01b9765f0;p=lhc%2Fweb%2Fwiklou.git

resourceloader: Only store sources' load.php urls

Previously ResourceLoader would store any arbitrary data about a
source, provided it had a 'loadScript' key. It would register
the 'local' source with an additional 'apiScript' key, which was
also documented in DefaultSettings.php. However, it was
completely unused outside of the ForeignAPIGadgetRepo class in
Gadgets 2.0, which should be changed to take an API url as a
parameter. This was not useful as it was not ever formally
exposed, and it could not be depended upon that a source had
registered an 'apiScript' key.

For backwards compatability, both ResourceLoader::addSource()
and mw.loader.addSource() will both take an array/object, but
discard all parameters except for 'loadScript'.

Also added tests for ResourceLoader::addSource().

Bug: 69878
Change-Id: I4205cf788cddeec13b619be0c3576197dec1b8bf
---

diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 14799c6464..a9b09e2962 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -3274,10 +3274,7 @@ $wgResourceModuleSkinStyles = array();
  *
  * @par Example:
  * @code
- *   $wgResourceLoaderSources['foo'] = array(
- *       'loadScript' => 'http://example.org/w/load.php',
- *       'apiScript' => 'http://example.org/w/api.php'
- *   );
+ *   $wgResourceLoaderSources['foo'] = 'http://example.org/w/load.php';
  * @endcode
  */
 $wgResourceLoaderSources = array();
diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index 5f72d8d2f4..a356e2e3b3 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -32,9 +32,6 @@ class ResourceLoader {
 	/** @var int */
 	protected static $filterCacheVersion = 7;
 
-	/** @var array */
-	protected static $requiredSourceProperties = array( 'loadScript' );
-
 	/** @var bool */
 	protected static $debugMode = null;
 
@@ -53,7 +50,7 @@ class ResourceLoader {
 	 */
 	protected $testModuleNames = array();
 
-	/** @var array E.g. array( 'source-id' => array( 'loadScript' => 'http://.../load.php' ) ) */
+	/** @var array E.g. array( 'source-id' => 'http://.../load.php' ) */
 	protected $sources = array();
 
 	/** @var bool */
@@ -231,10 +228,7 @@ class ResourceLoader {
 		$this->config = $config;
 
 		// Add 'local' source first
-		$this->addSource(
-			'local',
-			array( 'loadScript' => wfScript( 'load' ), 'apiScript' => wfScript( 'api' ) )
-		);
+		$this->addSource( 'local', wfScript( 'load' ) );
 
 		// Add other sources
 		$this->addSource( $config->get( 'ResourceLoaderSources' ) );
@@ -401,14 +395,12 @@ class ResourceLoader {
 	/**
 	 * Add a foreign source of modules.
 	 *
-	 * Source properties:
-	 * 'loadScript': URL (either fully-qualified or protocol-relative) of load.php for this source
-	 *
-	 * @param mixed $id Source ID (string), or array( id1 => props1, id2 => props2, ... )
-	 * @param array $properties Source properties
+	 * @param array|string $id Source ID (string), or array( id1 => loadUrl, id2 => loadUrl, ... )
+	 * @param string|array $loadUrl load.php url (string), or array with loadUrl key for
+	 *  backwards-compatability.
 	 * @throws MWException
 	 */
-	public function addSource( $id, $properties = null ) {
+	public function addSource( $id, $loadUrl = null ) {
 		// Allow multiple sources to be registered in one call
 		if ( is_array( $id ) ) {
 			foreach ( $id as $key => $value ) {
@@ -425,14 +417,18 @@ class ResourceLoader {
 			);
 		}
 
-		// Validate properties
-		foreach ( self::$requiredSourceProperties as $prop ) {
-			if ( !isset( $properties[$prop] ) ) {
-				throw new MWException( "Required property $prop missing from source ID $id" );
+		// Pre 1.24 backwards-compatability
+		if ( is_array( $loadUrl ) ) {
+			if ( !isset( $loadUrl['loadScript'] ) ) {
+				throw new MWException(
+					__METHOD__ . ' was passed an array with no "loadScript" key.'
+				);
 			}
+
+			$loadUrl = $loadUrl['loadScript'];
 		}
 
-		$this->sources[$id] = $properties;
+		$this->sources[$id] = $loadUrl;
 	}
 
 	/**
@@ -527,7 +523,7 @@ class ResourceLoader {
 	/**
 	 * Get the list of sources.
 	 *
-	 * @return array Like array( id => array of properties, .. )
+	 * @return array Like array( id => load.php url, .. )
 	 */
 	public function getSources() {
 		return $this->sources;
@@ -546,7 +542,7 @@ class ResourceLoader {
 		if ( !isset( $this->sources[$source] ) ) {
 			throw new MWException( "The $source source was never registered in ResourceLoader." );
 		}
-		return $this->sources[$source]['loadScript'];
+		return $this->sources[$source];
 	}
 
 	/**
@@ -1228,7 +1224,7 @@ class ResourceLoader {
 	 *   - ResourceLoader::makeLoaderSourcesScript( $id, $properties ):
 	 *       Register a single source
 	 *
-	 *   - ResourceLoader::makeLoaderSourcesScript( array( $id1 => $props1, $id2 => $props2, ... ) );
+	 *   - ResourceLoader::makeLoaderSourcesScript( array( $id1 => $loadUrl, $id2 => $loadUrl, ... ) );
 	 *       Register sources with the given IDs and properties.
 	 *
 	 * @param string $id Source ID
diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index d50fe48f94..0d57d59bb4 100644
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -628,12 +628,10 @@
 			 */
 			var registry = {},
 				//
-				// Mapping of sources, keyed by source-id, values are objects.
+				// Mapping of sources, keyed by source-id, values are strings.
 				// Format:
 				//	{
-				//		'sourceId': {
-				//			'loadScript': 'http://foo.bar/w/load.php'
-				//		}
+				//		'sourceId': 'http://foo.bar/w/load.php'
 				//	}
 				//
 				sources = {},
@@ -1478,7 +1476,7 @@
 
 					for ( source in splits ) {
 
-						sourceLoadScript = sources[source].loadScript;
+						sourceLoadScript = sources[source];
 
 						for ( group in splits[source] ) {
 
@@ -1552,15 +1550,14 @@
 				 *
 				 * The #work method will use this information to split up requests by source.
 				 *
-				 *     mw.loader.addSource( 'mediawikiwiki', { loadScript: '//www.mediawiki.org/w/load.php' } );
+				 *     mw.loader.addSource( 'mediawikiwiki', '//www.mediawiki.org/w/load.php' );
 				 *
 				 * @param {string} id Short string representing a source wiki, used internally for
 				 *  registered modules to indicate where they should be loaded from (usually lowercase a-z).
-				 * @param {Object} props
-				 * @param {string} props.loadScript Url to the load.php entry point of the source wiki.
+				 * @param {Object|string} loadUrl load.php url, may be an object for backwards-compatability
 				 * @return {boolean}
 				 */
-				addSource: function ( id, props ) {
+				addSource: function ( id, loadUrl ) {
 					var source;
 					// Allow multiple additions
 					if ( typeof id === 'object' ) {
@@ -1574,7 +1571,11 @@
 						throw new Error( 'source already registered: ' + id );
 					}
 
-					sources[id] = props;
+					if ( typeof loadUrl === 'object' ) {
+						loadUrl = loadUrl.loadScript;
+					}
+
+					sources[id] = loadUrl;
 
 					return true;
 				},
diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php
index 0c250bd762..a189387328 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php
@@ -9,10 +9,7 @@ class ResourceLoaderStartupModuleTest extends ResourceLoaderTestCase {
 				'modules' => array(),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    }
+    "local": "/w/load.php"
 } );mw.loader.register( [] );'
 			) ),
 			array( array(
@@ -22,10 +19,7 @@ mw.loader.addSource( {
 				),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    }
+    "local": "/w/load.php"
 } );mw.loader.register( [
     [
         "test.blank",
@@ -42,10 +36,7 @@ mw.loader.addSource( {
 				),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    }
+    "local": "/w/load.php"
 } );mw.loader.register( [
     [
         "test.blank",
@@ -73,10 +64,7 @@ mw.loader.addSource( {
 				),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    }
+    "local": "/w/load.php"
 } );mw.loader.register( [
     [
         "test.blank",
@@ -97,14 +85,8 @@ mw.loader.addSource( {
 				),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    },
-    "example": {
-        "loadScript": "http://example.org/w/load.php",
-        "apiScript": "http://example.org/w/api.php"
-    }
+    "local": "/w/load.php",
+    "example": "http://example.org/w/load.php"
 } );mw.loader.register( [
     [
         "test.blank",
@@ -140,10 +122,7 @@ mw.loader.addSource( {
 				),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    }
+    "local": "/w/load.php"
 } );mw.loader.register( [
     [
         "test.x.core",
@@ -238,14 +217,8 @@ mw.loader.addSource( {
 				),
 				'out' => '
 mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    },
-    "example": {
-        "loadScript": "http://example.org/w/load.php",
-        "apiScript": "http://example.org/w/api.php"
-    }
+    "local": "/w/load.php",
+    "example": "http://example.org/w/load.php"
 } );mw.loader.register( [
     [
         "test.blank",
@@ -369,7 +342,7 @@ mw.loader.addSource( {
 		$rl->register( $modules );
 		$module = new ResourceLoaderStartUpModule();
 		$this->assertEquals(
-'mw.loader.addSource({"local":{"loadScript":"/w/load.php","apiScript":"/w/api.php"}});'
+'mw.loader.addSource({"local":"/w/load.php"});'
 . 'mw.loader.register(['
 . '["test.blank","1388534400"],'
 . '["test.min","1388534400",["test.blank"],null,"local",'
@@ -390,10 +363,7 @@ mw.loader.addSource( {
 		$module = new ResourceLoaderStartUpModule();
 		$this->assertEquals(
 'mw.loader.addSource( {
-    "local": {
-        "loadScript": "/w/load.php",
-        "apiScript": "/w/api.php"
-    }
+    "local": "/w/load.php"
 } );mw.loader.register( [
     [
         "test.blank",
diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
index bd6b3f2329..81a3dc4b6f 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -131,6 +131,43 @@ class ResourceLoaderTest extends ResourceLoaderTestCase {
 		);
 	}
 
+	public static function provideAddSource() {
+		return array(
+			array( 'examplewiki', '//example.org/w/load.php', 'examplewiki' ),
+			array( 'example2wiki', array( 'loadScript' => '//example.com/w/load.php' ), 'example2wiki' ),
+			array(
+				array( 'foowiki' => '//foo.org/w/load.php', 'bazwiki' => '//baz.org/w/load.php' ),
+				null,
+				array( 'foowiki', 'bazwiki' )
+			),
+			array(
+				array( 'foowiki' => '//foo.org/w/load.php' ),
+				null,
+				false,
+			),
+		);
+	}
+
+	/**
+	 * @dataProvider provideAddSource
+	 * @covers ResourceLoader::addSource
+	 */
+	public function testAddSource( $name, $info, $expected ) {
+		$rl = new ResourceLoader;
+		if ( $expected === false ) {
+			$this->setExpectedException( 'MWException', 'ResourceLoader duplicate source addition error' );
+			$rl->addSource( $name, $info );
+		}
+		$rl->addSource( $name, $info );
+		if ( is_array( $expected ) ) {
+			foreach ( $expected as $source ) {
+				$this->assertArrayHasKey( $source, $rl->getSources() );
+			}
+		} else {
+			$this->assertArrayHasKey( $expected, $rl->getSources() );
+		}
+	}
+
 	public static function fakeSources() {
 		return array(
 			'examplewiki' => array(
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
index 96813305da..0767c9ff37 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -32,9 +32,7 @@
 
 	mw.loader.addSource(
 		'testloader',
-		{
-			loadScript: QUnit.fixurl( mw.config.get( 'wgScriptPath' ) + '/tests/qunit/data/load.mock.php' )
-		}
+		QUnit.fixurl( mw.config.get( 'wgScriptPath' ) + '/tests/qunit/data/load.mock.php' )
 	);
 
 	QUnit.test( 'Initial check', 8, function ( assert ) {