From b3d762e1480d1b896709174d0622d72e0952694d Mon Sep 17 00:00:00 2001
From: Tim Starling <tstarling@wikimedia.org>
Date: Mon, 11 May 2020 18:58:38 +1000
Subject: [PATCH] Add HookRegistry

Add a HookRegistry interface and two concrete implementations,
representing HookContainer's view of its environment. This simplifies
creation of a HookContainer for testing.

Add MediaWikiTestCaseTrait::createHookContainer() which can be used
in most of the places that were previously creating mock hook
containers. It can also replace setTemporaryHook() in some cases.

Change-Id: I9ce15591dc40b3d717c203fa973141aa45a2500c
---
 includes/HookContainer/GlobalHookRegistry.php | 37 ++++++++++++
 includes/HookContainer/HookContainer.php      | 50 ++++++++---------
 includes/HookContainer/HookRegistry.php       | 40 +++++++++++++
 includes/HookContainer/StaticHookRegistry.php | 51 +++++++++++++++++
 includes/ServiceWiring.php                    |  7 ++-
 tests/phpunit/MediaWikiTestCaseTrait.php      | 23 ++++++++
 .../HookContainer/HookContainerTest.php       | 56 +++++++------------
 7 files changed, 198 insertions(+), 66 deletions(-)
 create mode 100644 includes/HookContainer/GlobalHookRegistry.php
 create mode 100644 includes/HookContainer/HookRegistry.php
 create mode 100644 includes/HookContainer/StaticHookRegistry.php

diff --git a/includes/HookContainer/GlobalHookRegistry.php b/includes/HookContainer/GlobalHookRegistry.php
new file mode 100644
index 00000000000..961d7236ccf
--- /dev/null
+++ b/includes/HookContainer/GlobalHookRegistry.php
@@ -0,0 +1,37 @@
+<?php
+
+namespace MediaWiki\HookContainer;
+
+use ExtensionRegistry;
+
+/**
+ * A HookRegistry which sources its data from dynamically changing sources:
+ * $wgHooks and an ExtensionRegistry.
+ */
+class GlobalHookRegistry implements HookRegistry {
+	/** @var ExtensionRegistry */
+	private $extensionRegistry;
+	/** @var DeprecatedHooks */
+	private $deprecatedHooks;
+
+	public function __construct(
+		ExtensionRegistry $extensionRegistry,
+		DeprecatedHooks $deprecatedHooks
+	) {
+		$this->extensionRegistry = $extensionRegistry;
+		$this->deprecatedHooks = $deprecatedHooks;
+	}
+
+	public function getGlobalHooks() {
+		global $wgHooks;
+		return $wgHooks;
+	}
+
+	public function getExtensionHooks() {
+		return $this->extensionRegistry->getAttribute( 'Hooks' ) ?? [];
+	}
+
+	public function getDeprecatedHooks() {
+		return $this->deprecatedHooks;
+	}
+}
diff --git a/includes/HookContainer/HookContainer.php b/includes/HookContainer/HookContainer.php
index 48f9ad36cd0..7cb56cc7244 100644
--- a/includes/HookContainer/HookContainer.php
+++ b/includes/HookContainer/HookContainer.php
@@ -21,7 +21,6 @@
 namespace MediaWiki\HookContainer;
 
 use Closure;
-use ExtensionRegistry;
 use MWDebug;
 use MWException;
 use UnexpectedValueException;
@@ -45,15 +44,12 @@ class HookContainer implements SalvageableService {
 	/** @var array handler name and their handler objects */
 	private $handlersByName = [];
 
-	/** @var ExtensionRegistry */
-	private $extensionRegistry;
+	/** @var HookRegistry */
+	private $registry;
 
 	/** @var ObjectFactory */
 	private $objectFactory;
 
-	/** @var DeprecatedHooks */
-	private $deprecatedHooks;
-
 	/** @var int The next ID to be used by scopedRegister() */
 	private $nextScopedRegisterId = 0;
 
@@ -61,18 +57,15 @@ class HookContainer implements SalvageableService {
 	private $originalHooks;
 
 	/**
-	 * @param ExtensionRegistry $extensionRegistry
+	 * @param HookRegistry $hookRegistry
 	 * @param ObjectFactory $objectFactory
-	 * @param DeprecatedHooks $deprecatedHooks
 	 */
 	public function __construct(
-		ExtensionRegistry $extensionRegistry,
-		ObjectFactory $objectFactory,
-		DeprecatedHooks $deprecatedHooks
+		HookRegistry $hookRegistry,
+		ObjectFactory $objectFactory
 	) {
-		$this->extensionRegistry = $extensionRegistry;
+		$this->registry = $hookRegistry;
 		$this->objectFactory = $objectFactory;
-		$this->deprecatedHooks = $deprecatedHooks;
 	}
 
 	/**
@@ -120,7 +113,7 @@ class HookContainer implements SalvageableService {
 	public function run( string $hook, array $args = [], array $options = [] ) : bool {
 		$legacyHandlers = $this->getLegacyHandlers( $hook );
 		$options = array_merge(
-			$this->deprecatedHooks->getDeprecationInfo( $hook ) ?? [],
+			$this->registry->getDeprecatedHooks()->getDeprecationInfo( $hook ) ?? [],
 			$options
 		);
 		// Equivalent of legacy Hooks::runWithoutAbort()
@@ -328,10 +321,9 @@ class HookContainer implements SalvageableService {
 	 * @return bool Whether the hook has a handler registered to it
 	 */
 	public function isRegistered( string $hook ) : bool {
-		global $wgHooks;
-		$legacyRegisteredHook = !empty( $wgHooks[$hook] ) ||
+		$legacyRegisteredHook = !empty( $this->registry->getGlobalHooks()[$hook] ) ||
 			!empty( $this->legacyRegisteredHandlers[$hook] );
-		$registeredHooks = $this->extensionRegistry->getAttribute( 'Hooks' );
+		$registeredHooks = $this->registry->getExtensionHooks();
 		return !empty( $registeredHooks[$hook] ) || $legacyRegisteredHook;
 	}
 
@@ -342,11 +334,12 @@ class HookContainer implements SalvageableService {
 	 * @param callable|string|array $callback handler object to attach
 	 */
 	public function register( string $hook, $callback ) {
-		$deprecated = $this->deprecatedHooks->isHookDeprecated( $hook );
+		$deprecatedHooks = $this->registry->getDeprecatedHooks();
+		$deprecated = $deprecatedHooks->isHookDeprecated( $hook );
 		if ( $deprecated ) {
-			$deprecatedVersion = $this->deprecatedHooks->getDeprecationInfo( $hook )['deprecatedVersion']
-				?? false;
-			$component = $this->deprecatedHooks->getDeprecationInfo( $hook )['component'] ?? false;
+			$info = $deprecatedHooks->getDeprecationInfo( $hook );
+			$deprecatedVersion = $info['deprecatedVersion'] ?? false;
+			$component = $info['component'] ?? false;
 			wfDeprecated(
 				"$hook hook", $deprecatedVersion, $component
 			);
@@ -362,10 +355,9 @@ class HookContainer implements SalvageableService {
 	 * @return array function names
 	 */
 	public function getLegacyHandlers( string $hook ) : array {
-		global $wgHooks;
 		$handlers = array_merge(
 			$this->legacyRegisteredHandlers[$hook] ?? [],
-			$wgHooks[$hook] ?? []
+			$this->registry->getGlobalHooks()[$hook] ?? []
 		);
 		return $handlers;
 	}
@@ -378,14 +370,15 @@ class HookContainer implements SalvageableService {
 	 */
 	public function getHandlers( string $hook ) : array {
 		$handlers = [];
-		$registeredHooks = $this->extensionRegistry->getAttribute( 'Hooks' );
+		$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'];
 				// Skip hooks that both acknowledge deprecation and are deprecated in core
 				$flaggedDeprecated = !empty( $hookReference['deprecated'] );
-				$deprecated = $this->deprecatedHooks->isHookDeprecated( $hook );
+				$deprecated = $deprecatedHooks->isHookDeprecated( $hook );
 				if ( $deprecated && $flaggedDeprecated ) {
 					continue;
 				}
@@ -406,10 +399,11 @@ class HookContainer implements SalvageableService {
 	 * 2. an extension registers a handler in the new way but does not acknowledge deprecation
 	 */
 	public function emitDeprecationWarnings() {
-		$registeredHooks = $this->extensionRegistry->getAttribute( 'Hooks' ) ?? [];
+		$deprecatedHooks = $this->registry->getDeprecatedHooks();
+		$registeredHooks = $this->registry->getExtensionHooks();
 		foreach ( $registeredHooks as $name => $handlers ) {
-			if ( $this->deprecatedHooks->isHookDeprecated( $name ) ) {
-				$deprecationInfo = $this->deprecatedHooks->getDeprecationInfo( $name );
+			if ( $deprecatedHooks->isHookDeprecated( $name ) ) {
+				$deprecationInfo = $deprecatedHooks->getDeprecationInfo( $name );
 				$version = $deprecationInfo['deprecatedVersion'] ?? '';
 				$component = $deprecationInfo['component'] ?? 'MediaWiki';
 				foreach ( $handlers as $handler ) {
diff --git a/includes/HookContainer/HookRegistry.php b/includes/HookContainer/HookRegistry.php
new file mode 100644
index 00000000000..98bf2ab8667
--- /dev/null
+++ b/includes/HookContainer/HookRegistry.php
@@ -0,0 +1,40 @@
+<?php
+
+namespace MediaWiki\HookContainer;
+
+interface HookRegistry {
+	/**
+	 * Get the current contents of the $wgHooks variable or a mocked substitute
+	 * @return array
+	 */
+	public function getGlobalHooks();
+
+	/**
+	 * Get the current contents of the Hooks attribute in the ExtensionRegistry.
+	 * The contents is extended and normalized from the value of the
+	 * corresponding attribute in extension.json. It does not contain "legacy"
+	 * handlers, those are extracted into $wgHooks.
+	 *
+	 * It is a three dimensional array:
+	 *
+	 *   - The outer level is an array of hooks keyed by hook name.
+	 *   - The second level is an array of handlers, with integer indexes.
+	 *   - The third level is an associative array with the following members:
+	 *       - handler: An ObjectFactory spec, except that it also has an
+	 *         element "name" which is a unique string identifying the handler,
+	 *         for the purposes of sharing handler instances.
+	 *       - deprecated: A boolean value indicating whether the extension
+	 *         is acknowledging deprecation of the hook, to activate call
+	 *         filtering.
+	 *       - extensionPath: The path to the extension.json file in which the
+	 *         handler was defined. This is only used for deprecation messages.
+	 *
+	 * @return array
+	 */
+	public function getExtensionHooks();
+
+	/**
+	 * @return DeprecatedHooks
+	 */
+	public function getDeprecatedHooks();
+}
diff --git a/includes/HookContainer/StaticHookRegistry.php b/includes/HookContainer/StaticHookRegistry.php
new file mode 100644
index 00000000000..592514928ef
--- /dev/null
+++ b/includes/HookContainer/StaticHookRegistry.php
@@ -0,0 +1,51 @@
+<?php
+
+namespace MediaWiki\HookContainer;
+
+/**
+ * This is a simple immutable HookRegistry which can be used to set up a local
+ * HookContainer in tests and for similar purposes.
+ */
+class StaticHookRegistry implements HookRegistry {
+	/** @var array */
+	private $globalHooks;
+
+	/** @var array */
+	private $extensionHooks;
+
+	/** @var DeprecatedHooks */
+	private $deprecatedHooks;
+
+	/**
+	 * @param array $globalHooks An array of legacy hooks in the same format as $wgHooks
+	 * @param array $extensionHooks An array of modern hooks in the format
+	 *   described in HookRegistry::getExtensionHooks()
+	 * @param array $deprecatedHooksArray An array of deprecated hooks in the
+	 *   format expected by DeprecatedHooks::__construct(). These hooks are added
+	 *   to the core deprecated hooks list which is always present.
+	 */
+	public function __construct(
+		array $globalHooks = [],
+		array $extensionHooks = [],
+		array $deprecatedHooksArray = []
+	) {
+		$this->globalHooks = $globalHooks;
+		$this->extensionHooks = $extensionHooks;
+		$this->deprecatedHooks = new DeprecatedHooks( $deprecatedHooksArray );
+	}
+
+	public function getGlobalHooks() {
+		return $this->globalHooks;
+	}
+
+	public function getExtensionHooks() {
+		return $this->extensionHooks;
+	}
+
+	/**
+	 * @return DeprecatedHooks
+	 */
+	public function getDeprecatedHooks() {
+		return $this->deprecatedHooks;
+	}
+}
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index a3f84d42518..d5b8e88e5df 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -59,6 +59,7 @@ use MediaWiki\EditPage\SpamChecker;
 use MediaWiki\FileBackend\FSFile\TempFSFileFactory;
 use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory;
 use MediaWiki\HookContainer\DeprecatedHooks;
+use MediaWiki\HookContainer\GlobalHookRegistry;
 use MediaWiki\HookContainer\HookContainer;
 use MediaWiki\Http\HttpRequestFactory;
 use MediaWiki\Interwiki\ClassicInterwikiLookup;
@@ -352,10 +353,10 @@ return [
 		$extRegistry = ExtensionRegistry::getInstance();
 		$extDeprecatedHooks = $extRegistry->getAttribute( 'DeprecatedHooks' );
 		$deprecatedHooks = new DeprecatedHooks( $extDeprecatedHooks );
+		$hookRegistry = new GlobalHookRegistry( $extRegistry, $deprecatedHooks );
 		return new HookContainer(
-			$extRegistry,
-			$services->getObjectFactory(),
-			$deprecatedHooks
+			$hookRegistry,
+			$services->getObjectFactory()
 		);
 	},
 
diff --git a/tests/phpunit/MediaWikiTestCaseTrait.php b/tests/phpunit/MediaWikiTestCaseTrait.php
index 784ebf1a5b7..ddb369d775f 100644
--- a/tests/phpunit/MediaWikiTestCaseTrait.php
+++ b/tests/phpunit/MediaWikiTestCaseTrait.php
@@ -1,7 +1,10 @@
 <?php
 
+use MediaWiki\HookContainer\HookContainer;
 use PHPUnit\Framework\Constraint\Constraint;
 use PHPUnit\Framework\MockObject\MockObject;
+use Pimple\Psr11\ServiceLocator;
+use Wikimedia\ObjectFactory;
 
 /**
  * For code common to both MediaWikiUnitTestCase and MediaWikiIntegrationTestCase.
@@ -56,6 +59,26 @@ trait MediaWikiTestCaseTrait {
 		return $mock;
 	}
 
+	/**
+	 * Create an initially empty HookContainer with an empty service container
+	 * attached. Register only the hooks specified in the parameter.
+	 *
+	 * @param callable[] $hooks
+	 * @return HookContainer
+	 */
+	protected function createHookContainer( $hooks = [] ) {
+		$hookContainer = new HookContainer(
+			new \MediaWiki\HookContainer\StaticHookRegistry(),
+			new ObjectFactory(
+				new ServiceLocator( new \Pimple\Container(), [] )
+			)
+		);
+		foreach ( $hooks as $name => $callback ) {
+			$hookContainer->register( $name, $callback );
+		}
+		return $hookContainer;
+	}
+
 	/**
 	 * Don't throw a warning if $function is deprecated and called later
 	 *
diff --git a/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php b/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php
index 4114580a87f..a51b177125b 100644
--- a/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php
+++ b/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php
@@ -16,25 +16,19 @@ namespace MediaWiki\HookContainer {
 		 * Creates a new hook container with mocked ObjectFactory, ExtensionRegistry, and DeprecatedHooks
 		 */
 		private function newHookContainer(
-			$mockRegistry = null,
-			$objectFactory = null,
-			$deprecatedHooks = null
+			$hooks = null, $deprecatedHooksArray = []
 		) {
-			if ( !$mockRegistry ) {
+			if ( $hooks === null ) {
 				$handler = [ 'handler' => [
 					'name' => 'FooExtension-FooActionHandler',
 					'class' => 'FooExtension\\Hooks',
 					'services' => [] ]
 				];
-				$mockRegistry = $this->getMockExtensionRegistry( [ 'FooActionComplete' => [ $handler ] ] );
+				$hooks = [ 'FooActionComplete' => [ $handler ] ];
 			}
-			if ( !$objectFactory ) {
-				$objectFactory = $this->getObjectFactory();
-			}
-			if ( !$deprecatedHooks ) {
-				$deprecatedHooks = new DeprecatedHooks();
-			}
-			$hookContainer = new HookContainer( $mockRegistry, $objectFactory, $deprecatedHooks );
+			$mockObjectFactory = $this->getObjectFactory();
+			$registry = new StaticHookRegistry( [], $hooks, $deprecatedHooksArray );
+			$hookContainer = new HookContainer( $registry, $mockObjectFactory );
 			return $hookContainer;
 		}
 
@@ -233,12 +227,10 @@ namespace MediaWiki\HookContainer {
 			} else {
 				$hooks = [];
 			}
-			$mockExtensionRegistry = $this->getMockExtensionRegistry( $hooks );
 			$fakeDeprecatedHooks = [
 				'FooActionCompleteDeprecated' => [ 'deprecatedVersion' => '1.35' ]
 			];
-			$deprecatedHooks = new DeprecatedHooks( $fakeDeprecatedHooks );
-			$hookContainer = $this->newHookContainer( $mockExtensionRegistry, null, $deprecatedHooks );
+			$hookContainer = $this->newHookContainer( $hooks, $fakeDeprecatedHooks );
 			$handlers = $hookContainer->getHandlers( $hook );
 			$this->assertArrayEquals(
 				$handlers,
@@ -311,9 +303,7 @@ namespace MediaWiki\HookContainer {
 				'class' => 'FooExtension\\Hooks',
 				'services' => [] ]
 			];
-			$mockExtensionRegistry = $this->getMockExtensionRegistry(
-				[ 'InvalidReturnHandler' => [ $handler ] ] );
-			$hookContainer = $this->newHookContainer( $mockExtensionRegistry );
+			$hookContainer = $this->newHookContainer( [ 'InvalidReturnHandler' => [ $handler ] ] );
 			$this->expectException( UnexpectedValueException::class );
 			$this->expectExceptionMessage(
 				"Invalid return from onInvalidReturnHandler for " .
@@ -341,14 +331,14 @@ namespace MediaWiki\HookContainer {
 				'name' => 'FooExtension-Abort3',
 				'class' => 'FooExtension\\AbortHooks3'
 			] ];
-			$mockExtensionRegistry = $this->getMockExtensionRegistry( [
+			$hooks = [
 				'Abort' => [
 					$handler1,
 					$handler2,
 					$handler3
 				]
-			] );
-			$hookContainer = $this->newHookContainer( $mockExtensionRegistry );
+			];
+			$hookContainer = $this->newHookContainer( $hooks );
 			$called = [];
 			$ret = $hookContainer->run( 'Abort', [ &$called ] );
 			$this->assertFalse( $ret );
@@ -362,7 +352,6 @@ namespace MediaWiki\HookContainer {
 		public function testRegisterDeprecated() {
 			$this->hideDeprecated( 'FooActionComplete hook' );
 			$fakeDeprecatedHooks = [ 'FooActionComplete' => [ 'deprecatedVersion' => '1.0' ] ];
-			$deprecatedHooks = new DeprecatedHooks( $fakeDeprecatedHooks );
 			$handler = [
 				'handler' => [
 					'name' => 'FooExtension-FooActionHandler',
@@ -370,8 +359,9 @@ namespace MediaWiki\HookContainer {
 					'services' => []
 				]
 			];
-			$mockRegistry = $this->getMockExtensionRegistry( [ 'FooActionComplete' => [ $handler ] ] );
-			$hookContainer = $this->newHookContainer( $mockRegistry, null, $deprecatedHooks );
+			$hookContainer = $this->newHookContainer(
+				[ 'FooActionComplete' => [ $handler ] ],
+				$fakeDeprecatedHooks );
 			$hookContainer->register( 'FooActionComplete', new FooClass() );
 			$this->assertTrue( $hookContainer->isRegistered( 'FooActionComplete' ) );
 		}
@@ -400,9 +390,8 @@ namespace MediaWiki\HookContainer {
 				'class' => 'FooExtension\\Hooks',
 				'services' => [] ]
 			];
-			$mockExtensionRegistry = $this->getMockExtensionRegistry(
+			$hookContainer = $this->newHookContainer(
 				[ 'InvalidReturnHandler' => [ $handler ] ] );
-			$hookContainer = $this->newHookContainer( $mockExtensionRegistry );
 			$this->expectException( UnexpectedValueException::class );
 			$hookContainer->run( 'InvalidReturnHandler' );
 		}
@@ -411,22 +400,19 @@ namespace MediaWiki\HookContainer {
 		 * @covers       \MediaWiki\HookContainer\HookContainer::emitDeprecationWarnings
 		 */
 		public function testEmitDeprecationWarnings() {
-			$registry = $this->getMockExtensionRegistry( [
+			$hooks = [
 				'FooActionComplete' => [
 					[
 						'handler' => 'FooGlobalFunction',
 						'extensionPath' => 'fake-extension.json'
 					]
 				]
-			] );
+			];
+			$deprecatedHooksArray = [
+				'FooActionComplete' => [ 'deprecatedVersion' => '1.35' ]
+			];
 
-			$hookContainer = $this->newHookContainer(
-				$registry,
-				null,
-				new DeprecatedHooks( [
-					'FooActionComplete' => [ 'deprecatedVersion' => '1.35' ]
-				] )
-			);
+			$hookContainer = $this->newHookContainer( $hooks, $deprecatedHooksArray );
 
 			$this->expectDeprecation();
 			$hookContainer->emitDeprecationWarnings();