From: Kunal Mehta Date: Sat, 1 Aug 2015 07:38:27 +0000 (-0700) Subject: registration: Overhaul merging of globals X-Git-Tag: 1.31.0-rc.0~10548^2 X-Git-Url: http://git.cyclocoop.org/%24image?a=commitdiff_plain;h=1ebb0f5659667fd34c2c21ab2bb4068649d2ab07;p=lhc%2Fweb%2Fwiklou.git registration: Overhaul merging of globals Instead of hardcoding specific global settings in ExtensionRegistry, create specific "merge strategies" that are used to merge globals. Merge strategies are set for core properties in the ExtensionProcessor, and extensions can set them for their own configuration settings using the magic "_merge_strategy" key. The following merge strategies are included: * array_merge_recursive - call `array_merge_recursive` on the two arrays * array_plus - use the "+" operator to combine arrays, preserving integer keys * array_plus_2d - A version of array_plus that works on 2d arrays, used for merging arrays like $wgGroupPermissions * array_merge - call `array_merge` (default) This changes the merging of various namespaces related settings to use array_plus so they actually work. Bug: T107646 Change-Id: I64cb0553864e3b78b0f203333f58bb73b86a6434 --- diff --git a/docs/extension.schema.json b/docs/extension.schema.json index 8e6cafdb62..eef2c0ae14 100644 --- a/docs/extension.schema.json +++ b/docs/extension.schema.json @@ -626,7 +626,24 @@ }, "config": { "type": "object", - "description": "Configuration options for this extension" + "description": "Configuration options for this extension", + "patternProperties": { + "^[a-zA-Z_\u007f-\u00ff][a-zA-Z0-9_\u007f-\u00ff]*$": { + "type": ["object", "array", "string", "integer", "null", "boolean"], + "properties": { + "_merge_strategy": { + "type": "string", + "enum": [ + "array_merge_recursive", + "array_plus_2d", + "array_plus", + "array_merge" + ], + "default": "array_merge" + } + } + } + } }, "ParserTestFiles": { "type": "array", diff --git a/includes/registration/ExtensionProcessor.php b/includes/registration/ExtensionProcessor.php index 273e9ef0ba..da8600c77a 100644 --- a/includes/registration/ExtensionProcessor.php +++ b/includes/registration/ExtensionProcessor.php @@ -46,6 +46,24 @@ class ExtensionProcessor implements Processor { 'ValidSkinNames', ); + /** + * Mapping of global settings to their specific merge strategies. + * + * @see ExtensionRegistry::exportExtractedData + * @see getExtractedInfo + * @var array + */ + protected static $mergeStrategies = array( + 'wgGroupPermissions' => 'array_plus_2d', + 'wgRevokePermissions' => 'array_plus_2d', + 'wgHooks' => 'array_merge_recursive', + 'wgExtensionCredits' => 'array_merge_recursive', + 'wgExtraNamespaces' => 'array_plus', + 'wgExtraGenderNamespaces' => 'array_plus', + 'wgNamespacesWithSubpages' => 'array_plus', + 'wgNamespaceContentModels' => 'array_plus', + ); + /** * Keys that are part of the extension credits * @@ -156,6 +174,13 @@ class ExtensionProcessor implements Processor { } public function getExtractedInfo() { + // Make sure the merge strategies are set + foreach ( $this->globals as $key => $val ) { + if ( isset( self::$mergeStrategies[$key] ) ) { + $this->globals[$key][ExtensionRegistry::MERGE_STRATEGY] = self::$mergeStrategies[$key]; + } + } + return array( 'globals' => $this->globals, 'defines' => $this->defines, diff --git a/includes/registration/ExtensionRegistry.php b/includes/registration/ExtensionRegistry.php index c9df4b17df..7e113fc8c0 100644 --- a/includes/registration/ExtensionRegistry.php +++ b/includes/registration/ExtensionRegistry.php @@ -21,6 +21,13 @@ class ExtensionRegistry { */ const OLDEST_MANIFEST_VERSION = 1; + /** + * Special key that defines the merge strategy + * + * @since 1.26 + */ + const MERGE_STRATEGY = '_merge_strategy'; + /** * @var BagOStuff */ @@ -181,25 +188,54 @@ class ExtensionRegistry { protected function exportExtractedData( array $info ) { foreach ( $info['globals'] as $key => $val ) { + // If a merge strategy is set, read it and remove it from the value + // so it doesn't accidentally end up getting set. + // Need to check $val is an array for PHP 5.3 which will return + // true on isset( 'string'['foo'] ). + if ( isset( $val[self::MERGE_STRATEGY] ) && is_array( $val ) ) { + $mergeStrategy = $val[self::MERGE_STRATEGY]; + unset( $val[self::MERGE_STRATEGY] ); + } else { + $mergeStrategy = 'array_merge'; + } + + // Optimistic: If the global is not set, or is an empty array, replace it entirely. + // Will be O(1) performance. if ( !isset( $GLOBALS[$key] ) || ( is_array( $GLOBALS[$key] ) && !$GLOBALS[$key] ) ) { $GLOBALS[$key] = $val; - } elseif ( $key === 'wgHooks' || $key === 'wgExtensionCredits' ) { - // Special case $wgHooks and $wgExtensionCredits, which require a recursive merge. - // Ideally it would have been taken care of in the first if block though. - $GLOBALS[$key] = array_merge_recursive( $GLOBALS[$key], $val ); - } elseif ( $key === 'wgGroupPermissions' || $key === 'wgRevokePermissions' ) { - // First merge individual groups - foreach ( $GLOBALS[$key] as $name => &$groupVal ) { - if ( isset( $val[$name] ) ) { - $groupVal += $val[$name]; + continue; + } + + if ( !is_array( $GLOBALS[$key] ) || !is_array( $val ) ) { + // config setting that has already been overridden, don't set it + continue; + } + + switch ( $mergeStrategy ) { + case 'array_merge_recursive': + $GLOBALS[$key] = array_merge_recursive( $GLOBALS[$key], $val ); + break; + case 'array_plus_2d': + // First merge items that are in both arrays + foreach ( $GLOBALS[$key] as $name => &$groupVal ) { + if ( isset( $val[$name] ) ) { + $groupVal += $val[$name]; + } } - } - // Now merge groups that didn't exist yet - $GLOBALS[$key] += $val; - } elseif ( is_array( $GLOBALS[$key] ) && is_array( $val ) ) { - $GLOBALS[$key] = array_merge( $val, $GLOBALS[$key] ); - } // else case is a config setting where it has already been overriden, so don't set it + // Now add items that didn't exist yet + $GLOBALS[$key] += $val; + break; + case 'array_plus': + $GLOBALS[$key] = $val + $GLOBALS[$key]; + break; + case 'array_merge': + $GLOBALS[$key] = array_merge( $val, $GLOBALS[$key] ); + break; + default: + throw new UnexpectedValueException( "Unknown merge strategy '$mergeStrategy'" ); + } } + foreach ( $info['defines'] as $name => $val ) { define( $name, $val ); } diff --git a/tests/phpunit/includes/registration/ExtensionProcessorTest.php b/tests/phpunit/includes/registration/ExtensionProcessorTest.php index a79c9a878d..09641377da 100644 --- a/tests/phpunit/includes/registration/ExtensionProcessorTest.php +++ b/tests/phpunit/includes/registration/ExtensionProcessorTest.php @@ -38,6 +38,7 @@ class ExtensionProcessorTest extends MediaWikiTestCase { } public static function provideRegisterHooks() { + $merge = array( ExtensionRegistry::MERGE_STRATEGY => 'array_merge_recursive' ); // Format: // Current $wgHooks // Content in extension.json @@ -47,19 +48,19 @@ class ExtensionProcessorTest extends MediaWikiTestCase { array( array(), self::$default, - array(), + $merge, ), // No current hooks, adding one for "FooBaz" array( array(), array( 'Hooks' => array( 'FooBaz' => 'FooBazCallback' ) ) + self::$default, - array( 'FooBaz' => array( 'FooBazCallback' ) ), + array( 'FooBaz' => array( 'FooBazCallback' ) ) + $merge, ), // Hook for "FooBaz", adding another one array( array( 'FooBaz' => array( 'PriorCallback' ) ), array( 'Hooks' => array( 'FooBaz' => 'FooBazCallback' ) ) + self::$default, - array( 'FooBaz' => array( 'PriorCallback', 'FooBazCallback' ) ), + array( 'FooBaz' => array( 'PriorCallback', 'FooBazCallback' ) ) + $merge, ), // Hook for "BarBaz", adding one for "FooBaz" array( @@ -68,7 +69,7 @@ class ExtensionProcessorTest extends MediaWikiTestCase { array( 'BarBaz' => array( 'BarBazCallback' ), 'FooBaz' => array( 'FooBazCallback' ), - ), + ) + $merge, ), // Callbacks for FooBaz wrapped in an array array( @@ -76,7 +77,7 @@ class ExtensionProcessorTest extends MediaWikiTestCase { array( 'Hooks' => array( 'FooBaz' => array( 'Callback1' ) ) ) + self::$default, array( 'FooBaz' => array( 'Callback1' ), - ), + ) + $merge, ), // Multiple callbacks for FooBaz hook array( @@ -84,7 +85,7 @@ class ExtensionProcessorTest extends MediaWikiTestCase { array( 'Hooks' => array( 'FooBaz' => array( 'Callback1', 'Callback2' ) ) ) + self::$default, array( 'FooBaz' => array( 'Callback1', 'Callback2' ), - ), + ) + $merge, ), ); } diff --git a/tests/phpunit/includes/registration/ExtensionRegistryTest.php b/tests/phpunit/includes/registration/ExtensionRegistryTest.php index 515ce11fcb..b8b1b06922 100644 --- a/tests/phpunit/includes/registration/ExtensionRegistryTest.php +++ b/tests/phpunit/includes/registration/ExtensionRegistryTest.php @@ -101,6 +101,50 @@ class ExtensionRegistryTest extends MediaWikiTestCase { ), ) ), + array( + 'Global already set, 1d array that appends', + array( + 'mwAvailableRights' => array( + 'foobar', + 'foo' + ), + ), + array( + 'mwAvailableRights' => array( + 'barbaz', + ), + ), + array( + 'mwAvailableRights' => array( + 'barbaz', + 'foobar', + 'foo', + ), + ) + ), + array( + 'Global already set, 2d array with integer keys', + array( + 'mwNamespacesFoo' => array( + 100 => true, + 102 => false + ), + ), + array( + 'mwNamespacesFoo' => array( + 100 => false, + 500 => true, + ExtensionRegistry::MERGE_STRATEGY => 'array_plus', + ), + ), + array( + 'mwNamespacesFoo' => array( + 100 => false, + 102 => false, + 500 => true, + ), + ) + ), array( 'No global already set, $wgHooks', array( @@ -111,6 +155,7 @@ class ExtensionRegistryTest extends MediaWikiTestCase { 'FooBarEvent' => array( 'FooBarClass::onFooBarEvent' ), + ExtensionRegistry::MERGE_STRATEGY => 'array_merge_recursive' ), ), array( @@ -138,6 +183,7 @@ class ExtensionRegistryTest extends MediaWikiTestCase { 'FooBarEvent' => array( 'BazBarClass::onFooBarEvent', ), + ExtensionRegistry::MERGE_STRATEGY => 'array_merge_recursive', ), ), array( @@ -173,7 +219,8 @@ class ExtensionRegistryTest extends MediaWikiTestCase { 'right' => true, 'somethingtwo' => false, 'nonduplicated' => true, - ) + ), + ExtensionRegistry::MERGE_STRATEGY => 'array_plus_2d', ), ), array(