Fix behavior of Hooks class.

Until now, Hooks::run() would execute hooks registered via $wgHooks, but
Hooks:isRegistered() would not consider them and Hooks::getHandlers() would
not return them. That is inconsistent and misleading. This change aims to
make the methods of the Hooks class behave consistently, and allows them
to be used as a generic way of interacting with hooks.

Change-Id: I39bd5de2bc8ccbe8df729446363960af9d29b0be

diff --git a/includes/Hooks.php b/includes/Hooks.php
index 00dccdc..c9c0679 100644 (file)
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -39,12 +39,31 @@ class Hooks {
 
        protected static $handlers = array();
 
+       /**
+        * Clears hooks registered via Hooks::register(). Does not touch $wgHooks.
+        * This is intended for use while testing and will fail if MW_PHPUNIT_TEST is not defined.
+        *
+        * @since 1.21
+        *
+        * @param $name String: the name of the hook to clear.
+        *
+        * @throws MWException if not in testing mode.
+        */
+       public static function clear( $name ) {
+               if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
+                       throw new MWException( 'can not reset hooks in operation.' );
+               }
+
+               unset( self::$handlers[$name] );
+       }
+
+
        /**
         * Attach an event handler to a given hook
         *
         * @since 1.18
         *
-        * @param $name Mixed: name of hook
+        * @param $name String: name of hook
         * @param $callback Mixed: callback function to attach
         */
        public static function register( $name, $callback ) {
@@ -57,77 +76,81 @@ class Hooks {
 
        /**
         * Returns true if a hook has a function registered to it.
+        * The function may have been registered either via Hooks::register or in $wgHooks.
         *
         * @since 1.18
         *
-        * @param $name Mixed: name of hook
-        * @return Boolean: true if a hook has a function registered to it
+        * @param $name String: name of hook
+        * @return Boolean: true if the hook has a function registered to it
         */
        public static function isRegistered( $name ) {
-               if( !isset( self::$handlers[$name] ) ) {
-                       self::$handlers[$name] = array();
-               }
+               global $wgHooks;
 
-               return ( count( self::$handlers[$name] ) != 0 );
+               return !empty( $wgHooks[$name] ) || !empty( self::$handlers[$name] );
        }
 
        /**
         * Returns an array of all the event functions attached to a hook
-        *
+        * This combines functions registered via Hooks::register and with $wgHooks.
         * @since 1.18
         *
-        * @param $name Mixed: name of the hook
+        * @throws MWException
+        * @throws FatalError
+        * @param $name String: name of the hook
+        *
         * @return array
         */
        public static function getHandlers( $name ) {
-               if( !isset( self::$handlers[$name] ) ) {
+               global $wgHooks;
+
+               // Return quickly in the most common case
+               if ( empty( self::$handlers[$name] ) && empty( $wgHooks[$name] ) ) {
                        return array();
                }
 
-               return self::$handlers[$name];
+               if ( !is_array( self::$handlers ) ) {
+                       throw new MWException( "Local hooks array is not an array!\n" );
+               }
+
+               if ( !is_array( $wgHooks ) ) {
+                       throw new MWException( "Global hooks array is not an array!\n" );
+               }
+
+               if ( empty( Hooks::$handlers[$name] ) ) {
+                       $hooks = $wgHooks[$name];
+               } elseif ( empty( $wgHooks[$name] ) ) {
+                       $hooks = Hooks::$handlers[$name];
+               } else {
+                       // so they are both not empty...
+                       $hooks = array_merge( Hooks::$handlers[$name], $wgHooks[$name] );
+               }
+
+               if ( !is_array( $hooks ) ) {
+                       throw new MWException( "Hooks array for event '$name' is not an array!\n" );
+               }
+
+               return $hooks;
        }
 
        /**
         * Call hook functions defined in Hooks::register
         *
-        * Because programmers assign to $wgHooks, we need to be very
-        * careful about its contents. So, there's a lot more error-checking
-        * in here than would normally be necessary.
-        *
-        * @since 1.18
-        *
         * @param $event String: event name
-        * @param $args Array: parameters passed to hook functions
-        * @throws MWException
-        * @throws FatalError
+        * @param $args  Array: parameters passed to hook functions
+        *
         * @return Boolean True if no handler aborted the hook
         */
        public static function run( $event, $args = array() ) {
                global $wgHooks;
 
                // Return quickly in the most common case
-               if ( !isset( self::$handlers[$event] ) && !isset( $wgHooks[$event] ) ) {
+               if ( empty( self::$handlers[$event] ) && empty( $wgHooks[$event] ) ) {
                        return true;
                }
 
-               if ( !is_array( self::$handlers ) ) {
-                       throw new MWException( "Local hooks array is not an array!\n" );
-               }
-
-               if ( !is_array( $wgHooks ) ) {
-                       throw new MWException( "Global hooks array is not an array!\n" );
-               }
-
-               $new_handlers = (array) self::$handlers;
-               $old_handlers = (array) $wgHooks;
-
-               $hook_array = array_merge( $new_handlers, $old_handlers );
-
-               if ( !is_array( $hook_array[$event] ) ) {
-                       throw new MWException( "Hooks array for event '$event' is not an array!\n" );
-               }
+               $hooks = self::getHandlers( $event );
 
-               foreach ( $hook_array[$event] as $index => $hook ) {
+               foreach ( $hooks as $hook ) {
                        $object = null;
                        $method = null;
                        $func = null;
@@ -145,7 +168,7 @@ class Hooks {
                                if ( count( $hook ) < 1 ) {
                                        throw new MWException( 'Empty array in hooks for ' . $event . "\n" );
                                } elseif ( is_object( $hook[0] ) ) {
-                                       $object = $hook_array[$event][$index][0];
+                                       $object = $hook[0];
                                        if ( $object instanceof Closure ) {
                                                $closure = true;
                                                if ( count( $hook ) > 1 ) {
@@ -175,7 +198,7 @@ class Hooks {
                        } elseif ( is_string( $hook ) ) { # functions look like strings, too
                                $func = $hook;
                        } elseif ( is_object( $hook ) ) {
-                               $object = $hook_array[$event][$index];
+                               $object = $hook;
                                if ( $object instanceof Closure ) {
                                        $closure = true;
                                } else {

diff --git a/tests/phpunit/includes/HooksTest.php b/tests/phpunit/includes/HooksTest.php
index 2f9d9f8..e455f0f 100644 (file)
--- a/tests/phpunit/includes/HooksTest.php
+++ b/tests/phpunit/includes/HooksTest.php
@@ -75,27 +75,62 @@ class HooksTest extends MediaWikiTestCase {
 
                $this->assertEquals( 'bah', $foo, 'Standard static method' );
                $foo = 'Foo';
+
+               Hooks::clear( 'MediaWikiHooksTest001' );
+       }
+
+       public function testNewStyleHookInteraction() {
+               global $wgHooks;
+
+               $a = new NothingClass();
+               $b = new NothingClass();
+
+               // make sure to start with a clean slate
+               Hooks::clear( 'MediaWikiHooksTest001' );
+               unset( $wgHooks['MediaWikiHooksTest001'] );
+
+               $wgHooks['MediaWikiHooksTest001'][] = $a;
+               $this->assertTrue( Hooks::isRegistered( 'MediaWikiHooksTest001' ), 'Hook registered via $wgHooks should be noticed by Hooks::isRegistered' );
+
+               Hooks::register( 'MediaWikiHooksTest001', $b );
+               $this->assertEquals( 2, count( Hooks::getHandlers( 'MediaWikiHooksTest001' ) ), 'Hooks::getHandlers() should return hooks registered via wgHooks as well as Hooks::register' );
+
+               $foo = 'quux';
+               $bar = 'qaax';
+
+               Hooks::run( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
+               $this->assertEquals( 1, $a->calls, 'Hooks::run() should run hooks registered via wgHooks as well as Hooks::register' );
+               $this->assertEquals( 1, $b->calls, 'Hooks::run() should run hooks registered via wgHooks as well as Hooks::register' );
+
+               // clean up
+               Hooks::clear( 'MediaWikiHooksTest001' );
+               unset( $wgHooks['MediaWikiHooksTest001'] );
        }
 }
 
 class NothingClass {
+       public $calls = 0;
+
        static public function someStatic( &$foo, &$bar ) {
                $foo = 'bah';
                return true;
        }
 
        public function someNonStatic( &$foo, &$bar ) {
+               $this->calls++;
                $foo = 'fOO';
                $bar = 'bAR';
                return true;
        }
 
        public function onMediaWikiHooksTest001( &$foo, &$bar ) {
+               $this->calls++;
                $foo = 'foo';
                return true;
        }
 
        public function someNonStaticWithData( $foo, &$bar ) {
+               $this->calls++;
                $bar = $foo;
                return true;
        }