From f479715fd13fdb116eb6c0ce95725d31a683acad Mon Sep 17 00:00:00 2001
From: =?utf8?q?Niklas=20Laxstr=C3=B6m?= <nikerabbit@users.mediawiki.org>
Date: Wed, 28 Jul 2010 21:05:15 +0000
Subject: [PATCH] Better error message if hook function signature does not
 match parameters.

Also took the opportunity to write a short essay why this made me annoyed.
---
 includes/Hooks.php | 49 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/includes/Hooks.php b/includes/Hooks.php
index dec1c44236..01810b2b78 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -55,6 +55,7 @@ function wfRunHooks($event, $args = array()) {
 		$data = null;
 		$have_data = false;
 		$closure = false;
+		$badhookmsg = false;
 
 		/* $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
@@ -128,10 +129,34 @@ function wfRunHooks($event, $args = array()) {
 		// Run autoloader (workaround for call_user_func_array bug)
 		is_callable( $callback );
 
-		/* Call the hook. */
+		/* 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.
+		 */
+		$retval = null;
+		$handler = set_error_handler( 'hookErrorHandler' );
 		wfProfileIn( $func );
-		$retval = call_user_func_array( $callback, $hook_args );
+		try {
+			$retval = call_user_func_array( $callback, $hook_args );
+		} catch ( MWHookException $e ) {
+			$badhookmsg = $e->getMessage();
+		}
 		wfProfileOut( $func );
+		// Need to check for null, because set_error_handler borks on it... sigh
+		if ( $handler !== null ) set_error_handler( $handler );
 
 		/* String return is an error; false return means stop processing. */
 
@@ -152,9 +177,14 @@ function wfRunHooks($event, $args = array()) {
 			} else {
 				$prettyFunc = strval( $callback );
 			}
-			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." );
+			if ( $badhookmsg ) {
+				throw new MWException( "Detected bug in an extension! " .
+				"Hook $prettyFunc has invalid call signature; " . $badhookmsg );
+			} else {
+				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." );
+			}
 		} else if ( !$retval ) {
 			return false;
 		}
@@ -162,3 +192,12 @@ function wfRunHooks($event, $args = array()) {
 
 	return true;
 }
+
+function hookErrorHandler( $errno, $errstr ) {
+	if ( strpos( $errstr, 'expected to be a reference, value given' ) !== false ) {
+		throw new MWHookException( $errstr );
+	}
+	return false;
+}
+
+class MWHookException extends MWException {}
\ No newline at end of file
-- 
2.20.1