Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

HookContainer: allow registering broken callables.

Browse source 
Extensions may implement hooks defined by other extensions.
When the other extension is not present, the base class or interface of
the hook handler may also not be present. In that case, we should still
allow the handler to be registered. This is harmless, since the hook
will never fire if the extension that defines it is not loaded.

Bug: T339834
Change-Id: Id975370dececcb09ae5d00e6f68da57dc22fb267
This commit is contained in:
daniel 2023-06-19 20:48:28 +02:00 committed by Daniel Kinzler
parent d3dabf0cca
commit bf28ebd4ef
4 changed files with 160 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

48
includes/HookContainer/HookContainer.php
View file

@ -26,6 +26,7 @@
namespace MediaWiki\HookContainer;
use Closure;
use Error;
use InvalidArgumentException;
use LogicException;
use MWDebug;
@ -268,7 +269,7 @@ class HookContainer implements SalvageableService {
*
* @param string $hook Hook name
* @param string|array|callable $handler Executable handler function. See register() for supported structures.
* @param array $options
* @param array $options see makeExtensionHandlerCallback()
*
* @return array|false
* - callback: (callable) Executable handler function
@ -318,7 +319,7 @@ class HookContainer implements SalvageableService {
}
// Plain callback
if ( is_callable( $handler ) ) {
if ( self::mayBeCallable( $handler ) ) {
return [
'callback' => $handler,
'functionName' => self::callableToString( $handler ),
@ -383,7 +384,7 @@ class HookContainer implements SalvageableService {
wfDeprecatedMsg( "Deprecated handler style for hook '$hook': function wrapped in array ($msg)" );
}
if ( !is_callable( $callback ) ) {
if ( !self::mayBeCallable( $callback ) ) {
return false;
}
@ -419,6 +420,17 @@ class HookContainer implements SalvageableService {
* Several other forms are supported for backwards compatibility, but
* should not be used when calling this method directly.
*
* @note This method accepts "broken callables", that is, callable
* structures that reference classes that could not be found or could
* not be loaded, e.g. because they implement an interface that cannot
* be loaded. This situation may legitimately arise when implementing
* hooks defined by extensions that are not present.
* In that case, the hook will never fire and registering the "broken"
* handlers is harmless. If a broken hook handler is registered for a
* hook that is indeed called, it will cause an error. This is
* intentional: we don't want to silently ignore mistakes like mistyped
* class names in a hook handler registration.
*
* @param string $hook Name of hook
* @param string|array|callable $handler handler
*/
@ -688,4 +700,34 @@ class HookContainer implements SalvageableService {
$hook = strtr( $hook, ':\\-', '___' );
return "on$hook";
}
/**
* Replacement for is_callable that will also return true when the callable uses a class
* that cannot be loaded.
*
* This may legitimately happen when a hook handler uses a hook interfaces that is defined
* in another extension. In that case, the hook itself is also defined in the other extension,
* so the hook will never be called and no problem arises.
*
* However, it is entirely possible to register broken handlers for hooks that will indeed
* be called, causing an error. This is intentional: we don't want to silently ignore
* mistakes like mistyped class names in a hook handler registration.
*
* @param mixed $v
*
* @return bool
*/
private static function mayBeCallable( $v ): bool {
try {
return is_callable( $v );
} catch ( Error $error ) {
// If the callable uses a class that can't be loaded because it extends an unknown base class.
// Continue as if is_callable had returned true, to allow the handler to be registered.
if ( preg_match( '/Class.*not found/', $error->getMessage() ) ) {
return true;
}
throw $error;
}
}
}

2
tests/common/TestsAutoLoader.php
View file

@ -215,6 +215,8 @@ $wgAutoloadClasses += [
'NullGuzzleClient' => "$testDir/phpunit/mocks/NullGuzzleClient.php",
'NullHttpRequestFactory' => "$testDir/phpunit/mocks/NullHttpRequestFactory.php",
'NullMultiHttpClient' => "$testDir/phpunit/mocks/NullMultiHttpClient.php",
'MediaWiki\\Tests\\BrokenClass'
=> "$testDir/phpunit/mocks/BrokenClass.php",
'MediaWiki\\Tests\\Unit\\MockServiceDependenciesTrait'
=> "$testDir/phpunit/mocks/MockServiceDependenciesTrait.php",
'MediaWiki\\Tests\\Unit\\DummyServicesTrait'

14
tests/phpunit/mocks/BrokenClass.php Normal file
View file

@ -0,0 +1,14 @@
<?php
namespace MediaWiki\Tests;
/**
* A class that exists but cannot be instantiated because
* its base class does not exist. For testing logic related
* to class_exists and is_callable checks.
*/
class BrokenClass extends \Some\Thing\ThatDoesNotExist_8723465 {
public function aMethod() {
// noop
}
}

106
tests/phpunit/unit/includes/HookContainer/HookContainerTest.php
View file

@ -2,12 +2,14 @@
namespace MediaWiki\HookContainer {
use Error;
use InvalidArgumentException;
use MediaWiki\Tests\Unit\DummyServicesTrait;
use MediaWikiUnitTestCase;
use stdClass;
use UnexpectedValueException;
use Wikimedia\ScopedCallback;
use Wikimedia\TestingAccessWrapper;
class HookContainerTest extends MediaWikiUnitTestCase {
use DummyServicesTrait;
@ -61,11 +63,14 @@ namespace MediaWiki\HookContainer {
'function' => [ 'strtoupper', 'strtoupper' ],
'object' => [ new \FooExtension\Hooks(), 'FooExtension\Hooks::onFooActionComplete' ],
'object and method' => [ [ new FooClass(), 'fooMethod' ], 'MediaWiki\HookContainer\FooClass::fooMethod' ],
'object in array with no method' => [ new \FooExtension\Hooks(), 'FooExtension\Hooks::onFooActionComplete' ],
'extension' => [
self::HANDLER_REGISTRATION,
'FooExtension\Hooks::onFooActionComplete'
]
],
'callable referencing a class that extends an unknown class' => [
[ 'MediaWiki\Tests\BrokenClass', 'aMethod' ],
'MediaWiki\Tests\BrokenClass::aMethod'
],
];
}
@ -395,7 +400,7 @@ namespace MediaWiki\HookContainer {
],
'Object and fully-qualified non-static method' => [
[ $fooObj, 'MediaWiki\HookContainer\FooClass::fooMethod' ]
],
]
];
}
@ -784,7 +789,12 @@ namespace MediaWiki\HookContainer {
return false;
},
[ 'abortable' => false ]
]
],
'callable referencing a class that extends an unknown class' => [
[ 'MediaWiki\\Tests\\BrokenClass', 'aMethod' ],
[],
Error::class
],
];
}
@ -793,11 +803,12 @@ namespace MediaWiki\HookContainer {
* @covers \MediaWiki\HookContainer\HookContainer::normalizeHandler
* Test errors thrown with invalid handlers
*/
public function testRunErrors( $handler, $options ) {
public function testRunErrors( $handler, $options, $expected = UnexpectedValueException::class ) {
$hookContainer = $this->newHookContainer();
$this->filterDeprecated( '/^Returning a string from a hook handler/' );
$this->expectException( UnexpectedValueException::class );
$hookContainer->register( 'MWTestHook', $handler );
$this->filterDeprecated( '/^Returning a string from a hook handler/' );
$this->expectException( $expected );
$hookContainer->run( 'MWTestHook', [], $options );
}
@ -813,6 +824,10 @@ namespace MediaWiki\HookContainer {
'empty string' => [ '' ],
'zero' => [ 0 ],
'true' => [ true ],
'callable referencing an unknown class' => [
[ 'FooExtension\DoesNotExist', 'onFoo' ],
'FooExtension\DoesNotExist::onFoo'
],
];
}
@ -977,6 +992,83 @@ namespace MediaWiki\HookContainer {
$this->assertSame( 11, $count );
$this->assertTrue( $hookContainer->isRegistered( 'Increment' ) );
}
public static function provideMayBeCallable() {
yield 'function' => [
'strtoupper',
];
yield 'closure' => [
static function () {
// noop
},
];
yield 'object and method' => [
[ new FooClass(), 'fooMethod' ],
];
yield 'static method as array' => [
[ FooClass::class, 'fooStaticMethod', ],
];
yield 'static method as string' => [
'MediaWiki\HookContainer\FooClass::fooStaticMethod',
];
yield 'callable referencing a class that extends an unknown class' => [
[ 'MediaWiki\Tests\BrokenClass', 'aMethod' ],
];
}
public static function provideNotCallable() {
yield 'object' => [
new \FooExtension\Hooks(),
];
yield 'object and non-existing method' => [
[ new FooClass(), 'noSuchMethod' ],
];
yield 'object and method and extra stuff' => [
[ new FooClass(), 'fooMethod', 'extra', 'stuff' ],
];
yield 'object and method assoc' => [
[ 'a' => new FooClass(), 'b' => 'fooMethod' ],
];
yield 'object and method nested in array' => [
[ [ new FooClass(), 'fooMethod' ], 'whatever' ],
];
yield 'non-existing static method on existing class' => [
'MediaWiki\HookContainer\FooClass::noSuchMethod',
];
yield 'global function with extra data in array' => [
[ 'strtoupper', 'extra' ],
];
yield 'non-existing static method on existing class as array' => [
[ FooClass::class, 'noSuchMethod' ],
];
yield 'non-function text' => [
'just some text',
];
yield 'object in array with no method' => [
[ new \FooExtension\Hooks() ],
];
yield 'callable referencing an unknown class' => [
[ 'FooExtension\DoesNotExist', 'onFoo' ],
];
}
/**
* @covers \MediaWiki\HookContainer\HookContainer::mayBeCallable
* @dataProvider provideMayBeCallable
*/
public function testMayBeCallable_true( $v ) {
$access = TestingAccessWrapper::newFromClass( HookContainer::class );
$this->assertTrue( $access->mayBeCallable( $v ) );
}
/**
* @covers \MediaWiki\HookContainer\HookContainer::mayBeCallable
* @dataProvider provideNotCallable
*/
public function testMayBeCallable_false( $v ) {
$access = TestingAccessWrapper::newFromClass( HookContainer::class );
$this->assertFalse( $access->mayBeCallable( $v ) );
}
}
// Mock class for different types of handler functions