r80435 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r80434‎ | r80435 | r80436 >
Date:03:35, 17 January 2011
Author:soxred93
Status:ok (Comments)
Tags:
Comment:
Add new Hooks class, because $wgHooks globals are evil.
$wgHooks['EventName'][] = $callback; --> Hooks::register( 'EventName', $callback );
wfRunHooks( 'EventName', array() ); --> Hooks::run( 'EventName', array() );
Tests added to complement change. Backwards compatibility added.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/Hooks.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Hooks.php
@@ -35,172 +35,268 @@
3636 * @param $args Array: parameters passed to hook functions
3737 * @return Boolean
3838 */
39 -function wfRunHooks($event, $args = array()) {
 39+function wfRunHooks( $event, $args = array() ) {
 40+ return Hooks::run( $event, $args );
 41+}
4042
41 - global $wgHooks;
 43+function hookErrorHandler( $errno, $errstr ) {
 44+ return Hooks::hookErrorHandler( $errno, $errstr );
 45+}
4246
43 - // Return quickly in the most common case
44 - if ( !isset( $wgHooks[$event] ) ) {
45 - return true;
46 - }
 47+class MWHookException extends MWException {}
4748
48 - if (!is_array($wgHooks)) {
49 - throw new MWException("Global hooks array is not an array!\n");
50 - }
5149
52 - if (!is_array($wgHooks[$event])) {
53 - throw new MWException("Hooks array for event '$event' is not an array!\n");
 50+/**
 51+ * Hooks class.
 52+ *
 53+ * Used to supersede $wgHooks, because globals are EVIL.
 54+ *
 55+ */
 56+class Hooks {
 57+
 58+ protected static $handlers = array();
 59+
 60+ /**
 61+ * Attach an event handler to a given hook
 62+ *
 63+ * @access public
 64+ * @param mixed $name Name of hook
 65+ * @param mixed $callback Callback function to attach
 66+ * @return void
 67+ */
 68+ public static function register( $name, $callback ) {
 69+
 70+ if( !isset( self::$handlers[$name] ) ) {
 71+ self::$handlers[$name] = array();
 72+ }
 73+
 74+ self::$handlers[$name][] = $callback;
 75+
5476 }
55 -
56 - foreach ($wgHooks[$event] as $index => $hook) {
57 -
58 - $object = null;
59 - $method = null;
60 - $func = null;
61 - $data = null;
62 - $have_data = false;
63 - $closure = false;
64 - $badhookmsg = false;
65 -
66 - /* $hook can be: a function, an object, an array of $function and $data,
67 - * an array of just a function, an array of object and method, or an
68 - * array of object, method, and data.
69 - */
70 -
71 - if ( is_array( $hook ) ) {
72 - if ( count( $hook ) < 1 ) {
73 - throw new MWException("Empty array in hooks for " . $event . "\n");
74 - } else if ( is_object( $hook[0] ) ) {
75 - $object = $wgHooks[$event][$index][0];
76 - if ( $object instanceof Closure ) {
77 - $closure = true;
78 - if ( count( $hook ) > 1 ) {
 77+
 78+ /**
 79+ * Returns true if a hook has a function registered to it.
 80+ *
 81+ * @access public
 82+ * @param mixed $name Name of hook
 83+ * @return bool
 84+ */
 85+ public static function isRegistered( $name ) {
 86+
 87+ if( !isset( self::$handlers[$name] ) ) {
 88+ self::$handlers[$name] = array();
 89+ }
 90+
 91+ return ( count( self::$handlers[$name] ) != 0 );
 92+
 93+ }
 94+
 95+ /**
 96+ * Returns an array of all the event functions attached to a hook
 97+ *
 98+ * @access public
 99+ * @param mixed $name Name of hook
 100+ * @return array
 101+ */
 102+ public static function getHandlers( $name ) {
 103+
 104+ if( !isset( self::$handlers[$name] ) ) {
 105+ return array();
 106+ }
 107+
 108+ return self::$handlers[$name];
 109+
 110+ }
 111+
 112+
 113+
 114+
 115+ /**
 116+ * Call hook functions defined in Hooks::register
 117+ *
 118+ * Because programmers assign to $wgHooks, we need to be very
 119+ * careful about its contents. So, there's a lot more error-checking
 120+ * in here than would normally be necessary.
 121+ *
 122+ * @param $event String: event name
 123+ * @param $args Array: parameters passed to hook functions
 124+ * @return Boolean
 125+ */
 126+ public static function run( $event, $args = array() ) {
 127+
 128+ global $wgHooks;
 129+
 130+ // Return quickly in the most common case
 131+ if ( !isset( self::$handlers[$event] ) && !isset( $wgHooks[$event] ) ) {
 132+ return true;
 133+ }
 134+
 135+ if (!is_array(self::$handlers)) {
 136+ throw new MWException("Local hooks array is not an array!\n");
 137+ }
 138+
 139+ if (!is_array($wgHooks)) {
 140+ throw new MWException("Global hooks array is not an array!\n");
 141+ }
 142+
 143+ $new_handlers = (array) self::$handlers;
 144+ $old_handlers = (array) $wgHooks;
 145+
 146+ $hook_array = array_merge( $new_handlers, $old_handlers );
 147+
 148+ if ( !is_array($hook_array[$event]) ) {
 149+ throw new MWException("Hooks array for event '$event' is not an array!\n");
 150+ }
 151+
 152+ foreach ($hook_array[$event] as $index => $hook) {
 153+
 154+ $object = null;
 155+ $method = null;
 156+ $func = null;
 157+ $data = null;
 158+ $have_data = false;
 159+ $closure = false;
 160+ $badhookmsg = false;
 161+
 162+ /* $hook can be: a function, an object, an array of $function and $data,
 163+ * an array of just a function, an array of object and method, or an
 164+ * array of object, method, and data.
 165+ */
 166+
 167+ if ( is_array( $hook ) ) {
 168+ if ( count( $hook ) < 1 ) {
 169+ throw new MWException("Empty array in hooks for " . $event . "\n");
 170+ } else if ( is_object( $hook[0] ) ) {
 171+ $object = $hook_array[$event][$index][0];
 172+ if ( $object instanceof Closure ) {
 173+ $closure = true;
 174+ if ( count( $hook ) > 1 ) {
 175+ $data = $hook[1];
 176+ $have_data = true;
 177+ }
 178+ } else {
 179+ if ( count( $hook ) < 2 ) {
 180+ $method = "on" . $event;
 181+ } else {
 182+ $method = $hook[1];
 183+ if ( count( $hook ) > 2 ) {
 184+ $data = $hook[2];
 185+ $have_data = true;
 186+ }
 187+ }
 188+ }
 189+ } else if ( is_string( $hook[0] ) ) {
 190+ $func = $hook[0];
 191+ if ( count( $hook ) > 1) {
79192 $data = $hook[1];
80193 $have_data = true;
81194 }
82195 } else {
83 - if ( count( $hook ) < 2 ) {
84 - $method = "on" . $event;
85 - } else {
86 - $method = $hook[1];
87 - if ( count( $hook ) > 2 ) {
88 - $data = $hook[2];
89 - $have_data = true;
90 - }
91 - }
 196+ throw new MWException( "Unknown datatype in hooks for " . $event . "\n" );
92197 }
93 - } else if ( is_string( $hook[0] ) ) {
94 - $func = $hook[0];
95 - if ( count( $hook ) > 1) {
96 - $data = $hook[1];
97 - $have_data = true;
 198+ } else if ( is_string( $hook ) ) { # functions look like strings, too
 199+ $func = $hook;
 200+ } else if ( is_object( $hook ) ) {
 201+ $object = $hook_array[$event][$index];
 202+ if ( $object instanceof Closure ) {
 203+ $closure = true;
 204+ } else {
 205+ $method = "on" . $event;
98206 }
99207 } else {
100208 throw new MWException( "Unknown datatype in hooks for " . $event . "\n" );
101209 }
102 - } else if ( is_string( $hook ) ) { # functions look like strings, too
103 - $func = $hook;
104 - } else if ( is_object( $hook ) ) {
105 - $object = $wgHooks[$event][$index];
106 - if ( $object instanceof Closure ) {
107 - $closure = true;
 210+
 211+ /* We put the first data element on, if needed. */
 212+
 213+ if ( $have_data ) {
 214+ $hook_args = array_merge(array($data), $args);
108215 } else {
109 - $method = "on" . $event;
 216+ $hook_args = $args;
110217 }
111 - } else {
112 - throw new MWException( "Unknown datatype in hooks for " . $event . "\n" );
113 - }
114 -
115 - /* We put the first data element on, if needed. */
116 -
117 - if ( $have_data ) {
118 - $hook_args = array_merge(array($data), $args);
119 - } else {
120 - $hook_args = $args;
121 - }
122 -
123 - if ( $closure ) {
124 - $callback = $object;
125 - $func = "hook-$event-closure";
126 - } elseif ( isset( $object ) ) {
127 - $func = get_class( $object ) . '::' . $method;
128 - $callback = array( $object, $method );
129 - } elseif ( false !== ( $pos = strpos( $func, '::' ) ) ) {
130 - $callback = array( substr( $func, 0, $pos ), substr( $func, $pos + 2 ) );
131 - } else {
132 - $callback = $func;
133 - }
134 -
135 - // Run autoloader (workaround for call_user_func_array bug)
136 - is_callable( $callback );
137 -
138 - /* Call the hook. The documentation of call_user_func_array clearly
139 - * states that FALSE is returned on failure. However this is not
140 - * case always. In some version of PHP if the function signature
141 - * does not match the call signature, PHP will issue an warning:
142 - * Param y in x expected to be a reference, value given.
143 - *
144 - * In that case the call will also return null. The following code
145 - * catches that warning and provides better error message. The
146 - * function documentation also says that:
147 - * In other words, it does not depend on the function signature
148 - * whether the parameter is passed by a value or by a reference.
149 - * There is also PHP bug http://bugs.php.net/bug.php?id=47554 which
150 - * is unsurprisingly marked as bogus. In short handling of failures
151 - * with call_user_func_array is a failure, the documentation for that
152 - * function is wrong and misleading and PHP developers don't see any
153 - * problem here.
154 - */
155 - $retval = null;
156 - set_error_handler( 'hookErrorHandler' );
157 - wfProfileIn( $func );
158 - try {
159 - $retval = call_user_func_array( $callback, $hook_args );
160 - } catch ( MWHookException $e ) {
161 - $badhookmsg = $e->getMessage();
162 - }
163 - wfProfileOut( $func );
164 - restore_error_handler();
165 -
166 - /* String return is an error; false return means stop processing. */
167 - if ( is_string( $retval ) ) {
168 - global $wgOut;
169 - $wgOut->showFatalError( $retval );
170 - return false;
171 - } elseif( $retval === null ) {
 218+
172219 if ( $closure ) {
173 - $prettyFunc = "$event closure";
174 - } elseif( is_array( $callback ) ) {
175 - if( is_object( $callback[0] ) ) {
176 - $prettyClass = get_class( $callback[0] );
 220+ $callback = $object;
 221+ $func = "hook-$event-closure";
 222+ } elseif ( isset( $object ) ) {
 223+ $func = get_class( $object ) . '::' . $method;
 224+ $callback = array( $object, $method );
 225+ } elseif ( false !== ( $pos = strpos( $func, '::' ) ) ) {
 226+ $callback = array( substr( $func, 0, $pos ), substr( $func, $pos + 2 ) );
 227+ } else {
 228+ $callback = $func;
 229+ }
 230+
 231+ // Run autoloader (workaround for call_user_func_array bug)
 232+ is_callable( $callback );
 233+
 234+ /* Call the hook. The documentation of call_user_func_array clearly
 235+ * states that FALSE is returned on failure. However this is not
 236+ * case always. In some version of PHP if the function signature
 237+ * does not match the call signature, PHP will issue an warning:
 238+ * Param y in x expected to be a reference, value given.
 239+ *
 240+ * In that case the call will also return null. The following code
 241+ * catches that warning and provides better error message. The
 242+ * function documentation also says that:
 243+ * In other words, it does not depend on the function signature
 244+ * whether the parameter is passed by a value or by a reference.
 245+ * There is also PHP bug http://bugs.php.net/bug.php?id=47554 which
 246+ * is unsurprisingly marked as bogus. In short handling of failures
 247+ * with call_user_func_array is a failure, the documentation for that
 248+ * function is wrong and misleading and PHP developers don't see any
 249+ * problem here.
 250+ */
 251+ $retval = null;
 252+ set_error_handler( array( 'Hooks', 'hookErrorHandler' ) );
 253+ wfProfileIn( $func );
 254+ try {
 255+ $retval = call_user_func_array( $callback, $hook_args );
 256+ } catch ( MWHookException $e ) {
 257+ $badhookmsg = $e->getMessage();
 258+ }
 259+ wfProfileOut( $func );
 260+ restore_error_handler();
 261+
 262+ /* String return is an error; false return means stop processing. */
 263+ if ( is_string( $retval ) ) {
 264+ global $wgOut;
 265+ $wgOut->showFatalError( $retval );
 266+ return false;
 267+ } elseif( $retval === null ) {
 268+ if ( $closure ) {
 269+ $prettyFunc = "$event closure";
 270+ } elseif( is_array( $callback ) ) {
 271+ if( is_object( $callback[0] ) ) {
 272+ $prettyClass = get_class( $callback[0] );
 273+ } else {
 274+ $prettyClass = strval( $callback[0] );
 275+ }
 276+ $prettyFunc = $prettyClass . '::' . strval( $callback[1] );
177277 } else {
178 - $prettyClass = strval( $callback[0] );
 278+ $prettyFunc = strval( $callback );
179279 }
180 - $prettyFunc = $prettyClass . '::' . strval( $callback[1] );
181 - } else {
182 - $prettyFunc = strval( $callback );
 280+ if ( $badhookmsg ) {
 281+ throw new MWException( "Detected bug in an extension! " .
 282+ "Hook $prettyFunc has invalid call signature; " . $badhookmsg );
 283+ } else {
 284+ throw new MWException( "Detected bug in an extension! " .
 285+ "Hook $prettyFunc failed to return a value; " .
 286+ "should return true to continue hook processing or false to abort." );
 287+ }
 288+ } else if ( !$retval ) {
 289+ return false;
183290 }
184 - if ( $badhookmsg ) {
185 - throw new MWException( "Detected bug in an extension! " .
186 - "Hook $prettyFunc has invalid call signature; " . $badhookmsg );
187 - } else {
188 - throw new MWException( "Detected bug in an extension! " .
189 - "Hook $prettyFunc failed to return a value; " .
190 - "should return true to continue hook processing or false to abort." );
191 - }
192 - } else if ( !$retval ) {
193 - return false;
194291 }
 292+
 293+ return true;
195294 }
196 -
197 - return true;
198 -}
199 -
200 -function hookErrorHandler( $errno, $errstr ) {
201 - if ( strpos( $errstr, 'expected to be a reference, value given' ) !== false ) {
202 - throw new MWHookException( $errstr );
 295+
 296+ //This REALLY should be protected... but it's public for compatibility
 297+ public static function hookErrorHandler( $errno, $errstr ) {
 298+ if ( strpos( $errstr, 'expected to be a reference, value given' ) !== false ) {
 299+ throw new MWHookException( $errstr );
 300+ }
 301+ return false;
203302 }
204 - return false;
205303 }
206 -
207 -class MWHookException extends MWException {}
\ No newline at end of file
Index: trunk/phase3/includes/AutoLoader.php
@@ -105,6 +105,7 @@
106106 'HistoryBlobStub' => 'includes/HistoryBlob.php',
107107 'HistoryPage' => 'includes/HistoryPage.php',
108108 'HistoryPager' => 'includes/HistoryPage.php',
 109+ 'Hooks' => 'includes/Hooks.php',
109110 'Html' => 'includes/Html.php',
110111 'HTMLCacheUpdate' => 'includes/HTMLCacheUpdate.php',
111112 'HTMLCacheUpdateJob' => 'includes/HTMLCacheUpdate.php',
Index: trunk/phase3/RELEASE-NOTES
@@ -49,6 +49,7 @@
5050 * New hook ArticlePrepareTextForEdit added, called when preparing text to be saved.
5151 * New parser option PreSaveTransform added, allows the pre-save transformation
5252 to be selectively disabled.
 53+* Alternative to $wgHooks implemented, using the new Hooks class.
5354
5455 === Bug fixes in 1.18 ===
5556 * (bug 23119) WikiError class and subclasses are now marked as deprecated

Follow-up revisions

RevisionCommit summaryAuthorDate
r80445Followup to r80435: Forgot the svn addsoxred9316:43, 17 January 2011
r84555findhooks.php: also check for Hooks::run, not just wfRunHooks. Hooks class ha...ashley20:44, 22 March 2011

Comments

#Comment by Ilmari Karonen (talk | contribs)   04:33, 17 January 2011

What does this accomplish except adding overhead? I'm sorry, but I'm just not seeing the point. If there is some particular reason (besides slightly ugly syntax) why our existing hook infrastructure sucks and need to be changed, please enlighten me.

#Comment by X! (talk | contribs)   04:38, 17 January 2011

Mmm. Step 1 towards globalless code. It's not a mandatory change just yet.

#Comment by Reedy (talk | contribs)   06:01, 17 January 2011

Seem to also have excess whitespace between method, and inconsistent code style (ie not to our coding guidelines)

#Comment by Nikerabbit (talk | contribs)   07:33, 17 January 2011

+ * Used to supersede

Used to has certain meaning that is not appropriate here.

#Comment by X! (talk | contribs)   16:27, 17 January 2011

Correct, it should be "intended to".

#Comment by Hashar (talk | contribs)   12:54, 26 March 2011

Do we keep this class or revert it as well as the followup?

Marking fixme to attract reviewers.

#Comment by Happy-melon (talk | contribs)   13:00, 26 March 2011

I think it is a positive development; the fact that our existing infrastructure works 'adequately' is not a reason why there mightn't be a better system out there; indeed "adequate but could be better" applies to much of the codebase. Preserving B/C inevitably leads to slightly larger and/or heavier code, that's pretty much the definition of supporting two interfaces. I think the steady move towards global-less code (meaning both global functions and global variables) is productive, and worth some extra complexity in implementing.

#Comment by Jack Phoenix (talk | contribs)   11:30, 12 May 2011

I say we keep this class and deprecate $wgHooks and wfRunHooks() and remove them in the future (MW 1.20? 1.21?).

I'm being bold and setting the status to OK.

Status & tagging log