diff --git a/docs/Hooks.md b/docs/Hooks.md index a75269a12cb..8d2a6efe0ba 100644 --- a/docs/Hooks.md +++ b/docs/Hooks.md @@ -117,6 +117,31 @@ Then the extension will define a handler class: } } +## Service dependencies + +The ObjectFactory specification in HookHandlers can contain a list of services +which should be instantiated and provided to the constructor or factory +function for the handler. For example: + + "HookHandlers": { + "main": { + "class": "MediaWiki\\Extension\\FoodProcessor\\HookHandler", + "services": [ "ReadOnlyMode" ] + } + } + +However, care should be taken with this feature. Some services have expensive +constructors, so requesting them when handling commonly-called hooks may damage +performance. Also, some services may not be safe to construct from within a hook +call. + +The safest pattern for service injection is to use a separate handler for each +hook, and to inject only the services needed by that hook. + +Calling a hook with the `noServices` option disables service injection. If a +handler for such a hook specifies services, an exception will be thrown when +the hook is called. + ## Returning and aborting If a hook handler returns false, HookContainer will stop iterating through the diff --git a/includes/HookContainer/HookContainer.php b/includes/HookContainer/HookContainer.php index 57c3c7a4131..6837b6b440e 100644 --- a/includes/HookContainer/HookContainer.php +++ b/includes/HookContainer/HookContainer.php @@ -119,6 +119,7 @@ class HookContainer implements SalvageableService { * hooks to the DeprecatedHooks::$deprecatedHooks array literal. New extension code should * use the DeprecatedHooks attribute. * - silent: (bool) If true, do not raise a deprecation warning + * - noServices: (bool) If true, do not allow hook handlers with service dependencies * @return bool True if no handler aborted the hook * @throws UnexpectedValueException if handlers return an invalid value */ @@ -153,7 +154,7 @@ class HookContainer implements SalvageableService { } } - $handlers = $this->getHandlers( $hook ); + $handlers = $this->getHandlers( $hook, $options ); $funcName = 'on' . str_replace( ':', '_', ucfirst( $hook ) ); foreach ( $handlers as $handler ) { @@ -426,31 +427,38 @@ class HookContainer implements SalvageableService { * * @internal For use by Hooks.php * @param string $hook Name of the hook + * @param array $options Handler options, which may include: + * - noServices: Do not allow hook handlers with service dependencies * @return array non-deprecated handler objects */ - public function getHandlers( string $hook ) : array { + public function getHandlers( string $hook, array $options = [] ) : array { if ( $this->tombstones[$hook] ?? false ) { // There is at least one tombstone for the hook, so suppress all new-style hooks. return []; } - $handlers = []; $deprecatedHooks = $this->registry->getDeprecatedHooks(); $registeredHooks = $this->registry->getExtensionHooks(); if ( isset( $registeredHooks[$hook] ) ) { foreach ( $registeredHooks[$hook] as $hookReference ) { // Non-legacy hooks have handler attributes - $handlerObject = $hookReference['handler']; + $handlerSpec = $hookReference['handler']; // Skip hooks that both acknowledge deprecation and are deprecated in core $flaggedDeprecated = !empty( $hookReference['deprecated'] ); $deprecated = $deprecatedHooks->isHookDeprecated( $hook ); if ( $deprecated && $flaggedDeprecated ) { continue; } - $handlerName = $handlerObject['name']; + $handlerName = $handlerSpec['name']; + if ( !empty( $options['noServices'] ) && isset( $handlerSpec['services'] ) ) { + throw new UnexpectedValueException( + "The handler for the hook $hook registered in " . + "{$hookReference['extensionPath']} has a service dependency, " . + "but this hook does not allow it." ); + } if ( !isset( $this->handlersByName[$handlerName] ) ) { $this->handlersByName[$handlerName] = - $this->objectFactory->createObject( $handlerObject ); + $this->objectFactory->createObject( $handlerSpec ); } $handlers[] = $this->handlersByName[$handlerName]; } diff --git a/includes/HookContainer/HookRunner.php b/includes/HookContainer/HookRunner.php index b3d7d23e5fe..34122dd5b5b 100644 --- a/includes/HookContainer/HookRunner.php +++ b/includes/HookContainer/HookRunner.php @@ -2389,7 +2389,8 @@ class HookRunner implements public function onLoadExtensionSchemaUpdates( $updater ) { return $this->container->run( 'LoadExtensionSchemaUpdates', - [ $updater ] + [ $updater ], + [ 'noServices' => true ] ); } diff --git a/includes/installer/Hook/LoadExtensionSchemaUpdatesHook.php b/includes/installer/Hook/LoadExtensionSchemaUpdatesHook.php index c831cc7d49d..ac895b8df01 100644 --- a/includes/installer/Hook/LoadExtensionSchemaUpdatesHook.php +++ b/includes/installer/Hook/LoadExtensionSchemaUpdatesHook.php @@ -12,6 +12,10 @@ interface LoadExtensionSchemaUpdatesHook { /** * This hook is called during database installation and updates. * + * Do not use this hook with a handler that uses a "services" option in + * its ObjectFactory spec. It is called in a context where the global + * service locator is not initialised. + * * @since 1.35 * * @param DatabaseUpdater $updater DatabaseUpdater subclass diff --git a/tests/phpunit/includes/HookContainer/HookContainerIntegrationTest.php b/tests/phpunit/includes/HookContainer/HookContainerIntegrationTest.php index f8f0fc663c5..3e38677cc53 100644 --- a/tests/phpunit/includes/HookContainer/HookContainerIntegrationTest.php +++ b/tests/phpunit/includes/HookContainer/HookContainerIntegrationTest.php @@ -90,6 +90,47 @@ namespace MediaWiki\HookContainer { ScopedCallback::consume( $reset2 ); ScopedCallback::consume( $reset3 ); } + + /** + * @covers \MediaWiki\HookContainer\HookContainer + */ + public function testValidServiceInjection() { + $handler = [ + 'handler' => [ + 'name' => 'FooExtension-Mash', + 'class' => 'FooExtension\\ServiceHooks', + 'services' => [ 'ReadOnlyMode' ] + ], + 'extensionPath' => '/path/to/extension.json' + ]; + $hooks = [ 'Mash' => [ $handler ] ]; + $hookContainer = MediaWikiServices::getInstance()->getHookContainer(); + $reset = ExtensionRegistry::getInstance()->setAttributeForTest( 'Hooks', $hooks ); + $arg = 0; + $ret = $hookContainer->run( 'Mash', [ &$arg ] ); + $this->assertTrue( $ret ); + $this->assertSame( 1, $arg ); + } + + /** + * @covers \MediaWiki\HookContainer\HookContainer + */ + public function testInvalidServiceInjection() { + $handler = [ + 'handler' => [ + 'name' => 'FooExtension-Mash', + 'class' => 'FooExtension\\ServiceHooks', + 'services' => [ 'ReadOnlyMode' ] + ], + 'extensionPath' => '/path/to/extension.json' + ]; + $hooks = [ 'Mash' => [ $handler ] ]; + $hookContainer = MediaWikiServices::getInstance()->getHookContainer(); + $reset = ExtensionRegistry::getInstance()->setAttributeForTest( 'Hooks', $hooks ); + $this->expectException( \UnexpectedValueException::class ); + $arg = 0; + $hookContainer->run( 'Mash', [ &$arg ], [ 'noServices' => true ] ); + } } } @@ -102,4 +143,14 @@ namespace FooExtension { } } + class ServiceHooks { + public function __construct( \ReadOnlyMode $readOnlyMode ) { + } + + public function onMash( &$arg ) { + $arg++; + return true; + } + } + }