Krinkle has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/372594 )
Change subject: Hooks: Introduce NO_ABORT mode in Hooks::run()
......................................................................
Hooks: Introduce NO_ABORT mode in Hooks::run()
When set, any hook that returns something other than true or null,
will result in a run-time error.
To make it easier to introduce this opt-in flag, still allow explicit
'true' returns from existing callers.
Bug: T173615
Change-Id: I94c7ab656bd1a046410681a810c0a3fd4f72a2e5
---
M includes/Hooks.php
M tests/phpunit/includes/HooksTest.php
2 files changed, 83 insertions(+), 10 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/94/372594/1
diff --git a/includes/Hooks.php b/includes/Hooks.php
index f4f86be..5e03719 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -32,6 +32,9 @@
* @since 1.18
*/
class Hooks {
+ const ABORTABLE = 1;
+ const NO_ABORT = 3;
+
/**
* Array of events mapped to an array of callbacks to be run
* when that event is triggered.
@@ -118,7 +121,9 @@
*
* @param string $event Event name
* @param array $args Array of parameters passed to hook functions
- * @param string|null $deprecatedVersion Optionally, mark hook as
deprecated with version number
+ * @param string|null $deprecatedVersion [optional] Mark hook as
deprecated with version number
+ * @param int $flags [optional] Whether the hook is abortable. Use
Hooks::ABORTABLE or Hooks::NO_ABORT.
+ * Default: Hooks::ABORTABLE.
* @return bool True if no handler aborted the hook
*
* @throws Exception
@@ -128,7 +133,11 @@
* processing to continue. Not returning a value (or explicitly
* returning null) is equivalent to returning true.
*/
- public static function run( $event, array $args = [],
$deprecatedVersion = null ) {
+ public static function run( $event, array $args = [],
$deprecatedVersion = null, $flags = 0 ) {
+ if ( $flags === 0 && is_int( $deprecatedVersion ) ) {
+ $flags = $deprecatedVersion;
+ $deprecatedVersion = null;
+ }
foreach ( self::getHandlers( $event ) as $hook ) {
// Turn non-array values into an array. (Can't use
casting because of objects.)
if ( !is_array( $hook ) ) {
@@ -185,13 +194,17 @@
$hook_args = array_merge( $hook, $args );
$retval = call_user_func_array( $callback, $hook_args );
- // Process the return value.
- if ( is_string( $retval ) ) {
- // String returned means error.
- throw new FatalError( $retval );
- } elseif ( $retval === false ) {
- // False was returned. Stop processing, but no
error.
- return false;
+ if ( $flags === self::NO_ABORT && $retval !== null &&
$retval !== true ) {
+ wfLogWarning( "Invalid return from $func for
unabortable hook $event." );
+ } else {
+ // Process the return value.
+ if ( is_string( $retval ) ) {
+ // String returned means error.
+ throw new FatalError( $retval );
+ } elseif ( $retval === false ) {
+ // False was returned. Stop processing,
but no error.
+ return false;
+ }
}
}
diff --git a/tests/phpunit/includes/HooksTest.php
b/tests/phpunit/includes/HooksTest.php
index 527e129..0f75cf6 100644
--- a/tests/phpunit/includes/HooksTest.php
+++ b/tests/phpunit/includes/HooksTest.php
@@ -142,7 +142,67 @@
} );
$foo = 'original';
Hooks::run( 'MediaWikiHooksTest001', [ &$foo ] );
- $this->assertSame( 'original', $foo, 'Hooks continued
processing after a false return.' );
+ $this->assertSame( 'original', $foo, 'Hooks abort after a false
return.' );
+ }
+
+ /**
+ * @covers Hooks::run
+ */
+ public function testNoAbort() {
+ $list = [];
+ Hooks::register( 'MediaWikiHooksTest001', function ( &$list ) {
+ $list[] = 1;
+ return true; // Explicit true
+ } );
+ Hooks::register( 'MediaWikiHooksTest001', function ( &$list ) {
+ $list[] = 2;
+ return; // Implicit null
+ } );
+ Hooks::register( 'MediaWikiHooksTest001', function ( &$list ) {
+ $list[] = 3;
+ // No return
+ } );
+
+ MWDebug::init();
+ MWDebug::clearLog();
+ MediaWiki\suppressWarnings();
+ Hooks::run( 'MediaWikiHooksTest001', [ &$list ],
Hooks::NO_ABORT );
+ MediaWiki\restoreWarnings();
+ MWDebug::deinit();
+
+ $log = MWDebug::getLog();
+ $this->assertSame( [], $log, 'No warnings' );
+ $this->assertSame( [ 1, 2, 3 ], $list, 'All hooks ran.' );
+ }
+
+ /**
+ * @covers Hooks::run
+ */
+ public function testNoAbortWarning() {
+ Hooks::register( 'MediaWikiHooksTest001', function ( &$foo ) {
+ return false;
+ } );
+ Hooks::register( 'MediaWikiHooksTest001', function ( &$foo ) {
+ $foo = 'test';
+ return true;
+ } );
+ $foo = 'original';
+
+ MWDebug::init();
+ MWDebug::clearLog();
+ MediaWiki\suppressWarnings();
+ Hooks::run( 'MediaWikiHooksTest001', [ &$foo ], Hooks::NO_ABORT
);
+ MediaWiki\restoreWarnings();
+ MWDebug::deinit();
+
+ $log = MWDebug::getLog();
+ $this->assertSame( 'test', $foo, 'Unabortable hooks continue
after a false return.' );
+ $this->assertRegExp(
+ '/^Invalid return from .*closure for unabortable hook
MediaWikiHooksTest001.$/',
+ $log[0]['msg'],
+ 'Log message'
+ );
+ $this->assertSame( 'Hooks::run', $log[0]['caller'], 'Log
caller');
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/372594
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I94c7ab656bd1a046410681a810c0a3fd4f72a2e5
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <krinklem...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
