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 8e6cafd..eef2c0a 100644 (file)
--- 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 273e9ef..da8600c 100644 (file)
--- 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 c9df4b1..7e113fc 100644 (file)
--- 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 a79c9a8..0964137 100644 (file)
--- 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 515ce11..b8b1b06 100644 (file)
--- 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(