r102138 MediaWiki - Code Review archive

Hey there! This is an awesome improvement, I'm so glad somebody's helping me make AbuseFilter more extensible! I know the code isn't that easy to work with.

I know this is one of your first features, so I'll just give you a bit of feedback to help you get it all polished up.

+$wgAbuseFilterCustomActionsHandlers = false;

Why not just initialise to an empty array? The way you've done it, extensions have to have logic to check if it's been initialised yet. If you just set it to an empty array, this gets side-stepped.

+				if( is_array( $wgAbuseFilterCustomActionsHandlers ) &&
+					in_array( $action, array_keys( $wgAbuseFilterCustomActionsHandlers ) ) )
+				{

You can achieve the same result with array_key_exists( $action, $wgAbuseFilterCustomActionsHandlers)

+					if( is_callable( $custom_function ) ) {
+						$ok = call_user_func( $custom_function, $action, $parameters, $title, $vars, $rule_desc );
+					}

$ok won't be set if the function isn't callable. You should set it to false for the case that the function isn't callable (you might also want to consider throwing some sort of error in this case to improve debugging). This avoids a PHP error.

+				}
 				wfDebugLog( 'AbuseFilter', "Unrecognised action $action" );

You might want to make that debug logging appear in an 'else' clause, since the action isn't unrecognised if it's handled by your code.

One more recommendation: instead of using a specific message for the action, you might want to consider letting these custom functions return their own error message. So the function that gets put in $wgAbuseFilterCustomActionsHandlers can return whatever needs to be added to $display

Let me know how you go, and please do tell me if you need any help at all working with AbuseFilter.

Is there any reason to use array_key_exists over isset here?

Fixed in r102197.