From af125df5199b5c232bf335d735cecc6d8f8e9784 Mon Sep 17 00:00:00 2001
From: MatmaRex <matma.rex@gmail.com>
Date: Mon, 22 Apr 2013 16:38:47 +0200
Subject: [PATCH] Cleaned up Hooks code, comments, and documentation.

Essentially rewrote Hooks::run() to get rid of the ridiculous
four levels of indentation. Also made some slight adjustments
to fix rare edge cases (for example, moved set_error_handler
after wfProfileIn in case Profiler triggers an error).

Change-Id: Iafdd4ceedac067b49ac597359ac456f4617da9e8
---
 includes/Hooks.php                   | 287 ++++++++++-----------------
 tests/phpunit/includes/HooksTest.php | 156 ++++++++-------
 2 files changed, 188 insertions(+), 255 deletions(-)

diff --git a/includes/Hooks.php b/includes/Hooks.php
index 16fee4197f..5d8a4a7c57 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -1,4 +1,5 @@
 <?php
+
 /**
  * A tool for running hook functions.
  *
@@ -37,40 +38,43 @@ class MWHookException extends MWException {}
  */
 class Hooks {
 
+	/**
+	 * Array of events mapped to an array of callbacks to be run
+	 * when that event is triggered.
+	 */
 	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.
+	 * Attach an event handler to a given hook.
 	 *
-	 * @since 1.21
-	 *
-	 * @param string $name the name of the hook to clear.
+	 * @param string $name Name of hook
+	 * @param mixed $callback Callback function to attach
 	 *
-	 * @throws MWException if not in testing mode.
+	 * @since 1.18
 	 */
-	public static function clear( $name ) {
-		if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
-			throw new MWException( 'can not reset hooks in operation.' );
+	public static function register( $name, $callback ) {
+		if( !isset( self::$handlers[$name] ) ) {
+			self::$handlers[$name] = array();
 		}
 
-		unset( self::$handlers[$name] );
+		self::$handlers[$name][] = $callback;
 	}
 
 	/**
-	 * Attach an event handler to a given hook
+	 * 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.18
+	 * @param string $name the name of the hook to clear.
 	 *
-	 * @param string $name name of hook
-	 * @param $callback Mixed: callback function to attach
+	 * @since 1.21
+	 * @throws MWException if not in testing mode.
 	 */
-	public static function register( $name, $callback ) {
-		if ( !isset( self::$handlers[$name] ) ) {
-			self::$handlers[$name] = array();
+	public static function clear( $name ) {
+		if ( !defined( 'MW_PHPUNIT_TEST' ) ) {
+			throw new MWException( 'Cannot reset hooks in operation.' );
 		}
 
-		self::$handlers[$name][] = $callback;
+		unset( self::$handlers[$name] );
 	}
 
 	/**
@@ -79,221 +83,143 @@ class Hooks {
 	 *
 	 * @since 1.18
 	 *
-	 * @param string $name name of hook
-	 * @return Boolean: true if the hook has a function registered to it
+	 * @param string $name Name of hook
+	 * @return bool True if the hook has a function registered to it
 	 */
 	public static function isRegistered( $name ) {
 		global $wgHooks;
-
 		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
 	 *
-	 * @throws MWException
-	 * @throws FatalError
-	 * @param string $name name of the hook
+	 * @since 1.18
 	 *
+	 * @param string $name Name of the hook
 	 * @return array
 	 */
 	public static function getHandlers( $name ) {
 		global $wgHooks;
 
-		// Return quickly in the most common case
-		if ( empty( self::$handlers[$name] ) && empty( $wgHooks[$name] ) ) {
+		if ( !self::isRegistered( $name ) ) {
 			return array();
-		}
-
-		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];
+		} elseif ( !isset( self::$handlers[$name] ) ) {
+			return $wgHooks[$name];
+		} elseif ( !isset( $wgHooks[$name] ) ) {
+			return self::$handlers[$name];
 		} else {
-			// so they are both not empty...
-			$hooks = array_merge( Hooks::$handlers[$name], $wgHooks[$name] );
+			return array_merge( self::$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
+	 * Call hook functions defined in Hooks::register and $wgHooks.
 	 *
-	 * @param string $event event name
-	 * @param $args  Array: parameters passed to hook functions
+	 * For a certain hook event, fetch the array of hook events and
+	 * process them. Determine the proper callback for each hook and
+	 * then call the actual hook using the appropriate arguments.
+	 * Finally, process the return value and return/throw accordingly.
+	 *
+	 * @param string $event Event name
+	 * @param array $args  Array of parameters passed to hook functions
+	 * @return bool True if no handler aborted the hook
 	 *
 	 * @throws MWException
 	 * @throws FatalError
-	 * @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 ( empty( self::$handlers[$event] ) && empty( $wgHooks[$event] ) ) {
-			return true;
-		}
-
+	public static function run( $event, array $args = array() ) {
 		wfProfileIn( 'hook: ' . $event );
-		$hooks = self::getHandlers( $event );
+		foreach ( self::getHandlers( $event ) as $hook ) {
+			// Turn non-array values into an array. (Can't use casting because of objects.)
+			if ( !is_array( $hook ) ) {
+				$hook = array( $hook );
+			}
 
-		foreach ( $hooks as $hook ) {
-			$object = null;
-			$method = null;
-			$func = null;
-			$data = null;
-			$have_data = false;
-			$closure = false;
-			$badhookmsg = false;
+			if ( !array_filter( $hook ) ) {
+				// Either array is empty or it's an array filled with null/false/empty.
+				continue;
+			} elseif ( is_array( $hook[0] ) ) {
+				// First element is an array, meaning the developer intended
+				// the first element to be a callback. Merge it in so that
+				// processing can be uniform.
+				$hook = array_merge( $hook[0], array_slice( $hook, 1 ) );
+			}
 
 			/**
 			 * $hook can be: a function, an object, an array of $function and
 			 * $data, an array of just a function, an array of object and
 			 * method, or an array of object, method, and data.
 			 */
-			if ( is_array( $hook ) ) {
-				if ( count( $hook ) < 1 ) {
-					wfProfileOut( 'hook: ' . $event );
-					throw new MWException( 'Empty array in hooks for ' . $event . "\n" );
-				} elseif ( is_object( $hook[0] ) ) {
-					$object = $hook[0];
-					if ( $object instanceof Closure ) {
-						$closure = true;
-						if ( count( $hook ) > 1 ) {
-							$data = $hook[1];
-							$have_data = true;
-						}
-					} else {
-						if ( count( $hook ) < 2 ) {
-							$method = 'on' . $event;
-						} else {
-							$method = $hook[1];
-							if ( count( $hook ) > 2 ) {
-								$data = $hook[2];
-								$have_data = true;
-							}
-						}
-					}
-				} elseif ( is_string( $hook[0] ) ) {
-					$func = $hook[0];
-					if ( count( $hook ) > 1 ) {
-						$data = $hook[1];
-						$have_data = true;
-					}
-				} else {
-					wfProfileOut( 'hook: ' . $event );
-					throw new MWException( 'Unknown datatype in hooks for ' . $event . "\n" );
-				}
-			} elseif ( is_string( $hook ) ) { # functions look like strings, too
-				$func = $hook;
-			} elseif ( is_object( $hook ) ) {
-				$object = $hook;
-				if ( $object instanceof Closure ) {
-					$closure = true;
-				} else {
-					$method = "on" . $event;
+			if ( $hook[0] instanceof Closure ) {
+				$func = "hook-$event-closure";
+				$callback = array_shift( $hook );
+			} elseif ( is_object( $hook[0] ) ) {
+				$object = array_shift( $hook );
+				$method = array_shift( $hook );
+
+				// If no method was specified, default to on$event.
+				if ( $method === null ) {
+					$method = "on$event";
 				}
-			} else {
-				wfProfileOut( 'hook: ' . $event );
-				throw new MWException( 'Unknown datatype in hooks for ' . $event . "\n" );
-			}
-
-			/* We put the first data element on, if needed. */
-			if ( $have_data ) {
-				$hook_args = array_merge( array( $data ), $args );
-			} else {
-				$hook_args = $args;
-			}
 
-			if ( $closure ) {
-				$callback = $object;
-				$func = "hook-$event-closure";
-			} elseif ( isset( $object ) ) {
 				$func = get_class( $object ) . '::' . $method;
 				$callback = array( $object, $method );
+			} elseif ( is_string( $hook[0] ) ) {
+				$func = $callback = array_shift( $hook );
 			} else {
-				$callback = $func;
+				throw new MWException( 'Unknown datatype in hooks for ' . $event . "\n" );
 			}
 
 			// Run autoloader (workaround for call_user_func_array bug)
-			is_callable( $callback );
+			// and throw error if not callable.
+			if( !is_callable( $callback ) ) {
+				throw new MWException( 'Invalid callback in hooks for ' . $event . "\n" );
+			}
 
-			/**
-			 * Call the hook. The documentation of call_user_func_array clearly
-			 * states that FALSE is returned on failure. However this is not
-			 * case always. In some version of PHP if the function signature
-			 * does not match the call signature, PHP will issue an warning:
-			 * Param y in x expected to be a reference, value given.
-			 *
-			 * In that case the call will also return null. The following code
-			 * catches that warning and provides better error message. The
-			 * function documentation also says that:
-			 *     In other words, it does not depend on the function signature
-			 *     whether the parameter is passed by a value or by a reference.
-			 * There is also PHP bug http://bugs.php.net/bug.php?id=47554 which
-			 * is unsurprisingly marked as bogus. In short handling of failures
-			 * with call_user_func_array is a failure, the documentation for that
-			 * function is wrong and misleading and PHP developers don't see any
-			 * problem here.
+			/*
+			 * Call the hook. The documentation of call_user_func_array says
+			 * false is returned on failure. However, if the function signature
+			 * does not match the call signature, PHP will issue an warning and
+			 * return null instead. The following code catches that warning and
+			 * provides better error message.
 			 */
 			$retval = null;
-			set_error_handler( 'Hooks::hookErrorHandler' );
+			$badhookmsg = null;
+			$hook_args = array_merge( $hook, $args );
+
+			// Profile first in case the Profiler causes errors.
 			wfProfileIn( $func );
+			set_error_handler( 'Hooks::hookErrorHandler' );
 			try {
 				$retval = call_user_func_array( $callback, $hook_args );
 			} catch ( MWHookException $e ) {
 				$badhookmsg = $e->getMessage();
 			}
-			wfProfileOut( $func );
 			restore_error_handler();
+			wfProfileOut( $func );
 
-			/* String return is an error; false return means stop processing. */
+			// Process the return value.
 			if ( is_string( $retval ) ) {
+				// String returned means error.
 				throw new FatalError( $retval );
+			} elseif ( $badhookmsg !== null ) {
+				// Exception was thrown from Hooks::hookErrorHandler.
+				throw new MWException(
+					'Detected bug in an extension! ' .
+					"Hook $func has invalid call signature; " . $badhookmsg
+				);
 			} elseif ( $retval === null ) {
-				if ( $closure ) {
-					$prettyFunc = "$event closure";
-				} elseif ( is_array( $callback ) ) {
-					if ( is_object( $callback[0] ) ) {
-						$prettyClass = get_class( $callback[0] );
-					} else {
-						$prettyClass = strval( $callback[0] );
-					}
-					$prettyFunc = $prettyClass . '::' . strval( $callback[1] );
-				} else {
-					$prettyFunc = strval( $callback );
-				}
-				if ( $badhookmsg ) {
-					wfProfileOut( 'hook: ' . $event );
-					throw new MWException(
-						'Detected bug in an extension! ' .
-						"Hook $prettyFunc has invalid call signature; " . $badhookmsg
-					);
-				} else {
-					wfProfileOut( 'hook: ' . $event );
-					throw new MWException(
-						'Detected bug in an extension! ' .
-						"Hook $prettyFunc failed to return a value; " .
-						'should return true to continue hook processing or false to abort.'
-					);
-				}
+				// Null was returned. Error.
+				throw new MWException(
+					'Detected bug in an extension! ' .
+					"Hook $func failed to return a value; " .
+					'should return true to continue hook processing or false to abort.'
+				);
 			} elseif ( !$retval ) {
 				wfProfileOut( 'hook: ' . $event );
+				// False was returned. Stop processing, but no error.
 				return false;
 			}
 		}
@@ -303,18 +229,21 @@ class Hooks {
 	}
 
 	/**
+	 * Handle PHP errors issued inside a hook. Catch errors that have to do with
+	 * a function expecting a reference, and let all others pass through.
+	 *
 	 * This REALLY should be protected... but it's public for compatibility
 	 *
 	 * @since 1.18
 	 *
-	 * @param int $errno Unused
-	 * @param string $errstr error message
-	 * @throws MWHookException
-	 * @return Boolean: false
+	 * @param int $errno Error number (unused)
+	 * @param string $errstr Error message
+	 * @throws MWHookException If the error has to do with the function signature
+	 * @return bool Always returns false
 	 */
 	public static function hookErrorHandler( $errno, $errstr ) {
 		if ( strpos( $errstr, 'expected to be a reference, value given' ) !== false ) {
-			throw new MWHookException( $errstr );
+			throw new MWHookException( $errstr, $errno );
 		}
 		return false;
 	}
diff --git a/tests/phpunit/includes/HooksTest.php b/tests/phpunit/includes/HooksTest.php
index 89e789b1b2..8f70b3dfc0 100644
--- a/tests/phpunit/includes/HooksTest.php
+++ b/tests/phpunit/includes/HooksTest.php
@@ -2,81 +2,59 @@
 
 class HooksTest extends MediaWikiTestCase {
 
-	public function testOldStyleHooks() {
-		$foo = 'Foo';
-		$bar = 'Bar';
-
-		$i = new NothingClass();
-
+	function setUp() {
 		global $wgHooks;
-
-		$wgHooks['MediaWikiHooksTest001'][] = array( $i, 'someNonStatic' );
-
-		wfRunHooks( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
-
-		$this->assertEquals( 'fOO', $foo, 'Standard method' );
-		$foo = 'Foo';
-
-		$wgHooks['MediaWikiHooksTest001'][] = $i;
-
-		wfRunHooks( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
-
-		$this->assertEquals( 'foo', $foo, 'onEventName style' );
-		$foo = 'Foo';
-
-		$wgHooks['MediaWikiHooksTest001'][] = array( $i, 'someNonStaticWithData', 'baz' );
-
-		wfRunHooks( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
-
-		$this->assertEquals( 'baz', $foo, 'Data included' );
-		$foo = 'Foo';
-
-		$wgHooks['MediaWikiHooksTest001'][] = array( $i, 'someStatic' );
-
-		wfRunHooks( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
-
-		$this->assertEquals( 'bah', $foo, 'Standard static method' );
-		//$foo = 'Foo';
-
+		parent::setUp();
+		Hooks::clear( 'MediaWikiHooksTest001' );
 		unset( $wgHooks['MediaWikiHooksTest001'] );
-
 	}
 
-	public function testNewStyleHooks() {
-		$foo = 'Foo';
-		$bar = 'Bar';
-
+	public static function provideHooks() {
 		$i = new NothingClass();
+		return array(
+			array( 'Object and method', array( $i, 'someNonStatic' ), 'changed-nonstatic', 'changed-nonstatic' ),
+			array( 'Object and no method', array( $i ), 'changed-onevent', 'original' ),
+			array( 'Object and method with data', array( $i, 'someNonStaticWithData', 'data' ), 'data', 'original' ),
+			array( 'Object and static method', array( $i, 'someStatic' ), 'changed-static', 'original' ),
+			array( 'Class::method static call', array( 'NothingClass::someStatic' ), 'changed-static', 'original' ),
+			array( 'Global function', array( 'NothingFunction' ), 'changed-func', 'original' ),
+			array( 'Global function with data', array( 'NothingFunctionData', 'data' ), 'data', 'original' ),
+			array( 'Closure', array( function( &$foo, $bar ) {
+					$foo = 'changed-closure';
+					return true;
+				} ), 'changed-closure', 'original' ),
+			array( 'Closure with data', array( function( $data, &$foo, $bar ) {
+					$foo = $data;
+					return true;
+				}, 'data' ), 'data', 'original' )
+		);
+	}
 
-		Hooks::register( 'MediaWikiHooksTest001', array( $i, 'someNonStatic' ) );
-
-		Hooks::run( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
-
-		$this->assertEquals( 'fOO', $foo, 'Standard method' );
-		$foo = 'Foo';
-
-		Hooks::register( 'MediaWikiHooksTest001', $i );
-
-		Hooks::run( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
-
-		$this->assertEquals( 'foo', $foo, 'onEventName style' );
-		$foo = 'Foo';
-
-		Hooks::register( 'MediaWikiHooksTest001', array( $i, 'someNonStaticWithData', 'baz' ) );
+	/**
+	 * @dataProvider provideHooks
+	 */
+	public function testOldStyleHooks( $msg, array $hook, $expectedFoo, $expectedBar ) {
+		global $wgHooks;
+		$foo = $bar = 'original';
 
-		Hooks::run( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
+		$wgHooks['MediaWikiHooksTest001'][] = $hook;
+		wfRunHooks( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
 
-		$this->assertEquals( 'baz', $foo, 'Data included' );
-		$foo = 'Foo';
+		$this->assertSame( $expectedFoo, $foo, $msg );
+		$this->assertSame( $expectedBar, $bar, $msg );
+	}
 
-		Hooks::register( 'MediaWikiHooksTest001', array( $i, 'someStatic' ) );
+	/**
+	 * @dataProvider provideHooks
+	 */
+	public function testNewStyleHooks( $msg, $hook, $expectedFoo, $expectedBar ) {
+		$foo = $bar = 'original';
 
+		Hooks::register( 'MediaWikiHooksTest001', $hook );
 		Hooks::run( 'MediaWikiHooksTest001', array( &$foo, &$bar ) );
 
-		$this->assertEquals( 'bah', $foo, 'Standard static method' );
-		$foo = 'Foo';
-
-		Hooks::clear( 'MediaWikiHooksTest001' );
+		$this->assertSame( $expectedFoo, $foo, $msg );
+		$this->assertSame( $expectedBar, $bar, $msg );
 	}
 
 	public function testNewStyleHookInteraction() {
@@ -85,10 +63,6 @@ class HooksTest extends MediaWikiTestCase {
 		$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' );
 
@@ -101,37 +75,67 @@ class HooksTest extends MediaWikiTestCase {
 		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'] );
+	/**
+	 * @expectedException MWException
+	 */
+	public function testUncallableFunction() {
+		Hooks::register( 'MediaWikiHooksTest001', 'ThisFunctionDoesntExist' );
+		Hooks::run( 'MediaWikiHooksTest001', array() );
+	}
+
+	public function testFalseReturn() {
+		Hooks::register( 'MediaWikiHooksTest001', function( &$foo ) { return false; } );
+		Hooks::register( 'MediaWikiHooksTest001', function( &$foo ) { $foo = 'test'; return true; } );
+		$foo = 'original';
+		Hooks::run( 'MediaWikiHooksTest001', array( &$foo ) );
+		$this->assertSame( 'original', $foo, 'Hooks continued processing after a false return.' );
 	}
+
+	/**
+	 * @expectedException FatalError
+	 */
+	public function testFatalError() {
+		Hooks::register( 'MediaWikiHooksTest001', function() { return 'test'; } );
+		Hooks::run( 'MediaWikiHooksTest001', array() );
+	}
+}
+
+function NothingFunction( &$foo, &$bar ) {
+	$foo = 'changed-func';
+	return true;
+}
+
+function NothingFunctionData( $data, &$foo, &$bar ) {
+	$foo = $data;
+	return true;
 }
 
 class NothingClass {
 	public $calls = 0;
 
 	public static function someStatic( &$foo, &$bar ) {
-		$foo = 'bah';
+		$foo = 'changed-static';
 		return true;
 	}
 
 	public function someNonStatic( &$foo, &$bar ) {
 		$this->calls++;
-		$foo = 'fOO';
-		$bar = 'bAR';
+		$foo = 'changed-nonstatic';
+		$bar = 'changed-nonstatic';
 		return true;
 	}
 
 	public function onMediaWikiHooksTest001( &$foo, &$bar ) {
 		$this->calls++;
-		$foo = 'foo';
+		$foo = 'changed-onevent';
 		return true;
 	}
 
-	public function someNonStaticWithData( $foo, &$bar ) {
+	public function someNonStaticWithData( $data, &$foo, &$bar ) {
 		$this->calls++;
-		$bar = $foo;
+		$foo = $data;
 		return true;
 	}
 }
-- 
2.20.1