Prevent service injection to LoadExtensionSchemaUpdates hook

* Add noServices option to HookContainer::run()
* Use the option for LoadExtensionSchemaUpdates
* Document service injection including the caveats we know about

Bug: T258851
Change-Id: Ie57f2a3aeeea883f392e3c83ff228d1de68c6ebf

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

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];
 			}

includes/HookContainer/HookRunner.php

@@ -2389,7 +2389,8 @@ class HookRunner implements
 	public function onLoadExtensionSchemaUpdates( $updater ) {
 		return $this->container->run(
 			'LoadExtensionSchemaUpdates',
-			[ $updater ]
+			[ $updater ],
+			[ 'noServices' => true ]
 		);
 	}
 

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

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;
+		}
+	}
+
 }