Fix DeprecatedHooks and move emitDeprecatedHookWarnings to HookContainer

DeprecatedHooks was not listed as a core attribute and so was not extracted from extension.json. I added some code to extract and merge it in extractHooks(), and I also made the "component" default to the name of the extension which deprecates the hook, instead of "MediaWiki". I added the core deprecated hooks based on the current Hooks::run() calls. I moved emitDeprecatedHookWarnings() to HookContainer, and to reduce the performance overhead, arranged for it to be called only on page views. Change-Id: Idc0cfba782b49398d9e7feaa164fe7692d491bf9
2020-04-16 16:20:16 +10:00 · 2020-04-16 16:20:16 +10:00 · 6091dca5a6
commit 6091dca5a6
parent d506492d26
6 changed files with 198 additions and 138 deletions
--- a/includes/HookContainer/DeprecatedHooks.php
+++ b/includes/HookContainer/DeprecatedHooks.php
@ -26,6 +26,34 @@ use InvalidArgumentException;

 class DeprecatedHooks {

+	/**
+	 * @var array[] List of deprecated hooks. Value arrays for each hook contain:
+	 *  - deprecatedVersion: (string) Version in which the hook was deprecated,
+	 *    to pass to wfDeprecated().
+	 *  - component: (string, optional) $component to pass to wfDeprecated().
+	 */
+	private $deprecatedHooks = [
+		'APIQueryInfoTokens' => [ 'deprecatedVersion' => '1.24' ],
+		'APIQueryRecentChangesTokens' => [ 'deprecatedVersion' => '1.24' ],
+		'APIQueryRevisionsTokens' => [ 'deprecatedVersion' => '1.24' ],
+		'APIQueryUsersTokens' => [ 'deprecatedVersion' => '1.24' ],
+		'ApiTokensGetTokenTypes' => [ 'deprecatedVersion' => '1.24' ],
+		'ArticleEditUpdates' => [ 'deprecatedVersion' => '1.35' ],
+		'ArticleEditUpdatesDeleteFromRecentchanges' => [ 'deprecatedVersion' => '1.35' ],
+		'ArticleRevisionUndeleted' => [ 'deprecatedVersion' => '1.35' ],
+		'BeforeParserrenderImageGallery' => [ 'deprecatedVersion' => '1.35' ],
+		'InternalParseBeforeSanitize' => [ 'deprecatedVersion' => '1.35' ],
+		'LinkBegin' => [ 'deprecatedVersion' => '1.28' ],
+		'LinkEnd' => [ 'deprecatedVersion' => '1.28' ],
+		'ParserBeforeTidy' => [ 'deprecatedVersion' => '1.35' ],
+		'ParserFetchTemplate' => [ 'deprecatedVersion' => '1.35' ],
+		'ParserGetVariableValueVarCache' => [ 'deprecatedVersion' => '1.35' ],
+		'ParserPreSaveTransformComplete' => [ 'deprecatedVersion' => '1.35' ],
+		'ParserSectionCreate' => [ 'deprecatedVersion' => '1.35' ],
+		'RevisionInsertComplete' => [ 'deprecatedVersion' => '1.31' ],
+		'UndeleteShowRevision' => [ 'deprecatedVersion' => '1.35' ],
+	];
+
 	/**
 	 * @param array[] $deprecatedHooks List of hooks to mark as deprecated.
 	 * Value arrays for each hook contain:
@ -39,14 +67,6 @@ class DeprecatedHooks {
 		}
 	}

-	/**
-	 * @var array[] List of deprecated hooks. Value arrays for each hook contain:
-	 *  - deprecatedVersion: (string) Version in which the hook was deprecated,
-	 *    to pass to wfDeprecated().
-	 *  - component: (string, optional) $component to pass to wfDeprecated().
-	 */
-	private $deprecatedHooks = [];
-
 	/**
 	 * For use by extensions, to add to list of deprecated hooks.
 	 * Core-defined hooks should instead be added to $this->$deprecatedHooks directly.
--- a/includes/HookContainer/HookContainer.php
+++ b/includes/HookContainer/HookContainer.php
@ -22,6 +22,7 @@ namespace MediaWiki\HookContainer;

 use Closure;
 use ExtensionRegistry;
+use MWDebug;
 use MWException;
 use UnexpectedValueException;
 use Wikimedia\Assert\Assert;
@ -107,7 +108,9 @@ class HookContainer implements SalvageableService {
 	 *   - abortable: (bool) If false, handlers will not be allowed to abort the call sequenece.
 	 *     An exception will be raised if a handler returns anything other than true or null.
 	 *   - deprecatedVersion: (string) Version of MediaWiki this hook was deprecated in. For supporting
-	 *     Hooks::run() legacy $deprecatedVersion parameter
+	 *     Hooks::run() legacy $deprecatedVersion parameter. New core code should add deprecated
+	 *     hooks to the DeprecatedHooks::$deprecatedHooks array literal. New extension code should
+	 *     use the DeprecatedHooks attribute.
 	 * @return bool True if no handler aborted the hook
 	 * @throws UnexpectedValueException if handlers return an invalid value
 	 */
@ -361,4 +364,28 @@ class HookContainer implements SalvageableService {
 		}
 		return $handlers;
 	}
+
+	/**
+	 * Will log a deprecation warning if:
+	 * 1. the hook is marked deprecated
+	 * 2. an extension registers a handler in the new way but does not acknowledge deprecation
+	 */
+	public function emitDeprecationWarnings() {
+		$registeredHooks = $this->extensionRegistry->getAttribute( 'Hooks' ) ?? [];
+		foreach ( $registeredHooks as $name => $handlers ) {
+			if ( $this->deprecatedHooks->isHookDeprecated( $name ) ) {
+				$deprecationInfo = $this->deprecatedHooks->getDeprecationInfo( $name );
+				$version = $deprecationInfo['deprecatedVersion'] ?? '';
+				$component = $deprecationInfo['component'] ?? 'MediaWiki';
+				foreach ( $handlers as $handler ) {
+					if ( !isset( $handler['deprecated'] ) || !$handler['deprecated'] ) {
+						MWDebug::sendRawDeprecated(
+							"Hook $name was deprecated in $component $version " .
+							"but is registered in " . $handler['extensionPath']
+						);
+					}
+				}
+			}
+		}
+	}
 }
--- a/includes/actions/ViewAction.php
+++ b/includes/actions/ViewAction.php
@ -18,6 +18,8 @@
 * @ingroup Actions
 */

+use MediaWiki\MediaWikiServices;
+
 /**
 * An action that views article content
 *
@ -38,6 +40,12 @@ class ViewAction extends FormlessAction {
 	public function show() {
 		$config = $this->context->getConfig();

+		// Emit deprecated hook warnings.
+		// We do this only in the view action so that it reliably shows up in
+		// the debug toolbar without unduly impacting the performance of API and
+		// ResourceLoader requests.
+		MediaWikiServices::getInstance()->getHookContainer()->emitDeprecationWarnings();
+
 		if (
 			$config->get( 'DebugToolbar' ) == false && // don't let this get stuck on pages
 			$this->getWikiPage()->checkTouched() // page exists and is not a redirect
--- a/includes/registration/ExtensionProcessor.php
+++ b/includes/registration/ExtensionProcessor.php
@ -1,7 +1,5 @@
 <?php

-use MediaWiki\HookContainer\DeprecatedHooks;
-
 class ExtensionProcessor implements Processor {

 	/**
@ -275,39 +273,7 @@ class ExtensionProcessor implements Processor {
 		}
 	}

-	/**
-	 * Will throw wfDeprecated() warning if:
-	 * 1. an extension does not acknowledge deprecation and
-	 * 2. is marked deprecated
-	 */
-	private function emitDeprecatedHookWarnings() {
-		if ( !isset( $this->attributes['Hooks'] ) ) {
-			return;
-		}
-		$extDeprecatedHooks = $this->attributes['DeprecatedHooks'] ?? false;
-		if ( !$extDeprecatedHooks ) {
-			return;
-		}
-		$deprecatedHooks = new DeprecatedHooks( $extDeprecatedHooks );
-		foreach ( $this->attributes['Hooks'] as $name => $handlers ) {
-			if ( $deprecatedHooks->isHookDeprecated( $name ) ) {
-				$deprecationInfo = $deprecatedHooks->getDeprecationInfo( $name );
-				foreach ( $handlers as $handler ) {
-					if ( !isset( $handler['deprecated'] ) || !$handler['deprecated'] ) {
-						wfDeprecated(
-							"$name hook",
-							$deprecationInfo['deprecatedVersion'] ?? false,
-							$deprecationInfo['component'] ?? false
-						);
-					}
-				}
-			}
-		}
-	}
-
 	public function getExtractedInfo() {
-		$this->emitDeprecatedHookWarnings();
-
 		// Make sure the merge strategies are set
 		foreach ( $this->globals as $key => $val ) {
 			if ( isset( self::MERGE_STRATEGIES[$key] ) ) {
@ -438,6 +404,7 @@ class ExtensionProcessor implements Processor {
 				);
 			}
 			$callback['handler'] = $handlerDefinition;
+			$callback['extensionPath'] = $path;
 			$this->attributes['Hooks'][$name][] = $callback;
 		} else {
 			foreach ( $callback as $callable ) {
@ -448,7 +415,7 @@ class ExtensionProcessor implements Processor {
 						$this->globals['wgHooks'][$name][] = $callable;
 					}
 				} elseif ( is_string( $callable ) ) {
-					$this->setStringHookHandler( $callable, $hookHandlersAttr, $name );
+					$this->setStringHookHandler( $callable, $hookHandlersAttr, $name, $path );
 				}
 			}
 		}
@ -462,14 +429,19 @@ class ExtensionProcessor implements Processor {
 	 * @param string $callback Handler
 	 * @param array $hookHandlersAttr handler definitions from 'HookHandler' attribute
 	 * @param string $name
+	 * @param string $path
 	 */
 	private function setStringHookHandler(
 		string $callback,
 		array $hookHandlersAttr,
-		string $name
+		string $name,
+		string $path
 	) {
 		if ( isset( $hookHandlersAttr[$callback] ) ) {
-			$handler = [ 'handler' => $hookHandlersAttr[$callback] ];
+			$handler = [
+				'handler' => $hookHandlersAttr[$callback],
+				'extensionPath' => $path
+			];
 			$this->attributes['Hooks'][$name][] = $handler;
 		} else { // legacy style handler
 			$this->globals['wgHooks'][$name][] = $callback;
@ -485,18 +457,30 @@ class ExtensionProcessor implements Processor {
 	 * @param string $path path to extension.json
 	 */
 	protected function extractHooks( array $info, string $path ) {
-		if ( !isset( $info['Hooks'] ) ) {
-			return;
+		$extName = $info['name'];
+		if ( isset( $info['Hooks'] ) ) {
+			$hookHandlersAttr = [];
+			foreach ( $info['HookHandlers'] ?? [] as $name => $def ) {
+				$hookHandlersAttr[$name] = [ 'name' => "$extName-$name" ] + $def;
+			}
+			foreach ( $info['Hooks'] as $name => $callback ) {
+				if ( is_string( $callback ) ) {
+					$this->setStringHookHandler( $callback, $hookHandlersAttr, $name, $path );
+				} elseif ( is_array( $callback ) ) {
+					$this->setArrayHookHandler( $callback, $hookHandlersAttr, $name, $path );
+				}
+			}
 		}
-		$hookHandlersAttr = [];
-		foreach ( $info['HookHandlers'] ?? [] as $name => $def ) {
-			$hookHandlersAttr[$name] = [ 'name' => "$path-$name" ] + $def;
-		}
-		foreach ( $info['Hooks'] as $name => $callback ) {
-			if ( is_string( $callback ) ) {
-				$this->setStringHookHandler( $callback, $hookHandlersAttr, $name );
-			} elseif ( is_array( $callback ) ) {
-				$this->setArrayHookHandler( $callback, $hookHandlersAttr, $name, $path );
+		if ( isset( $info['DeprecatedHooks'] ) ) {
+			$deprecatedHooks = [];
+			foreach ( $info['DeprecatedHooks'] as $name => $info ) {
+				$info += [ 'component' => $extName ];
+				$deprecatedHooks[$name] = $info;
+			}
+			if ( isset( $this->attributes['DeprecatedHooks'] ) ) {
+				$this->attributes['DeprecatedHooks'] += $deprecatedHooks;
+			} else {
+				$this->attributes['DeprecatedHooks'] = $deprecatedHooks;
 			}
 		}
 	}
--- a/tests/phpunit/includes/registration/ExtensionProcessorTest.php
+++ b/tests/phpunit/includes/registration/ExtensionProcessorTest.php
@ -122,32 +122,6 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 		$this->assertArrayNotHasKey( 123456, $extracted['globals']['wgNamespacesWithSubpages'] );
 	}

-	public function provideDeprecatedNonLegacyHooks() {
-		// Format:
-		// Current Hooks attribute
-		// Content in extension.json
-		return [
-			[
-				[ 'ExtensionOwnedFooBaz' => [
-					[
-						'handler' => [
-							'class' => 'FooClass',
-							'services' => [],
-							'name' => $this->getCurrentDir() . '-PriorCallback',
-						]
-					]
-				] ],
-				[
-					'Hooks' => [ 'ExtensionOwnedFooBaz' => [ [ 'handler' => 'HandlerObjectCallback' ] ] ],
-					'HookHandlers' => [
-						'HandlerObjectCallback' => [ 'class' => 'FooClass', 'services' => [] ]
-					],
-					'DeprecatedHooks' => [ 'ExtensionOwnedFooBaz' => [ 'deprecatedVersion' => '1.0' ] ]
-				] + self::$default,
-			],
-		];
-	}
-
 	public function provideMixedStyleHooks() {
 		// Format:
 		// Content in extension.json
@ -179,21 +153,31 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 				] + [ ExtensionRegistry::MERGE_STRATEGY => 'array_merge_recursive' ],
 				[
 					'FooBaz' => [
-						[ 'handler' => [
-							'class' => 'FooClass',
-							'services' => [],
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback'
-						] ],
-						[ 'handler' => [
-							'class' => 'FooClass',
-							'services' => [],
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback'
-						], 'deprecated' => true ],
-						[ 'handler' => [
-							'class' => 'FooClass',
-							'services' => [],
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback'
-						] ]
+						[
+							'handler' => [
+								'class' => 'FooClass',
+								'services' => [],
+								'name' => 'FooBar-HandlerObjectCallback'
+							],
+							'extensionPath' => $this->getCurrentDir()
+						],
+						[
+							'handler' => [
+								'class' => 'FooClass',
+								'services' => [],
+								'name' => 'FooBar-HandlerObjectCallback'
+							],
+							'deprecated' => true,
+							'extensionPath' => $this->getCurrentDir()
+						],
+						[
+							'handler' => [
+								'class' => 'FooClass',
+								'services' => [],
+								'name' => 'FooBar-HandlerObjectCallback'
+							],
+							'extensionPath' => $this->getCurrentDir()
+						]
 					]
 				]
 			]
@ -215,7 +199,7 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 						'HandlerObjectCallback' => [
 							'class' => 'FooClass',
 							'services' => [],
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback'
+							'name' => 'FooBar-HandlerObjectCallback'
 						]
 					]
 				] + self::$default,
@ -226,9 +210,10 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 							'handler' => [
 								'class' => 'FooClass',
 								'services' => [],
-								'name' => $this->getCurrentDir() . '-HandlerObjectCallback'
+								'name' => 'FooBar-HandlerObjectCallback'
 							],
-							'deprecated' => true
+							'deprecated' => true,
+							'extensionPath' => $this->getCurrentDir()
 						]
 					]
 				],
@ -246,11 +231,14 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 				[ 'FooBaz' =>
 					[
 						'PriorCallback',
-						[ 'handler' => [
-							'class' => 'FooClass',
-							'services' => [],
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback'
-						] ],
+						[
+							'handler' => [
+								'class' => 'FooClass',
+								'services' => [],
+								'name' => 'FooBar-HandlerObjectCallback'
+							],
+							'extensionPath' => $this->getCurrentDir()
+						],
 					]
 				],
 				[]
@ -271,16 +259,23 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 				[ 'FooBaz' =>
 					[
 						'PriorCallback',
-						[ 'handler' => [
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback',
-							'class' => 'FooClass',
-							'services' => []
-						], 'deprecated' => true ],
-						[ 'handler' => [
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback2',
-							'class' => 'FooClass',
-							'services' => [],
-						] ]
+						[
+							'handler' => [
+								'name' => 'FooBar-HandlerObjectCallback',
+								'class' => 'FooClass',
+								'services' => []
+							],
+							'deprecated' => true,
+							'extensionPath' => $this->getCurrentDir()
+						],
+						[
+							'handler' => [
+								'name' => 'FooBar-HandlerObjectCallback2',
+								'class' => 'FooClass',
+								'services' => [],
+							],
+							'extensionPath' => $this->getCurrentDir()
+						]
 					]
 				],
 				[]
@ -301,11 +296,14 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 				[ 'FooBaz' =>
 					[
 						'PriorCallback',
-						[ 'handler' => [
-							'name' => $this->getCurrentDir() . '-HandlerObjectCallback',
-							'class' => 'FooClass',
-							'services' => []
-						] ],
+						[
+							'handler' => [
+								'name' => 'FooBar-HandlerObjectCallback',
+								'class' => 'FooClass',
+								'services' => []
+							],
+							'extensionPath' => $this->getCurrentDir()
+						],
 					]
 				],
 				[ 'FooClass', 'FooMethod' ]
@ -372,17 +370,6 @@ class ExtensionProcessorTest extends MediaWikiTestCase {
 		];
 	}

-	/**
-	 * @dataProvider provideDeprecatedNonLegacyHooks
-	 */
-	public function testDeprecatedNonLegacyHooks( $pre, $info ) {
-		// Use Case: Marking a hook deprecated that has already been loaded by another extension
-		$processor = new MockExtensionProcessor( [ 'attributes' => [ 'Hooks' => $pre ] ] );
-		$this->expectDeprecation();
-		$processor->extractInfo( $this->dir, $info, 1 );
-		$processor->getExtractedInfo();
-	}
-
 	/**
 	 * @dataProvider provideNonLegacyHooks
 	 */
--- a/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php
+++ b/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php
@ -37,8 +37,20 @@ namespace MediaWiki\HookContainer {
 			return $hookContainer;
 		}

-		private function getMockDeprecatedHooks() {
+		private function getMockDeprecatedHooks( $deprecatedHookInfo = null ) {
 			$mockDeprecatedHooks = $this->createMock( DeprecatedHooks::class );
+			if ( $deprecatedHookInfo ) {
+				foreach ( $deprecatedHookInfo as $name => $info ) {
+					$mockDeprecatedHooks
+						->method( 'isHookDeprecated' )
+						->with( $name )
+						->willReturn( true );
+					$mockDeprecatedHooks
+						->method( 'getDeprecationInfo' )
+						->with( $name )
+						->willReturn( $info );
+				}
+			}
 			return $mockDeprecatedHooks;
 		}

@ -325,11 +337,9 @@ namespace MediaWiki\HookContainer {
 		 */
 		public function testRegisterDeprecated() {
 			$this->hideDeprecated( 'FooActionComplete hook' );
-			$mockDeprecatedHooks = $this->getMockDeprecatedHooks();
-			$mockDeprecatedHooks->method( 'isHookDeprecated' )->with( 'FooActionComplete' )
-				->willReturn( true );
-			$mockDeprecatedHooks->method( 'getDeprecationInfo' )->with( 'FooActionComplete' )
-				->willReturn( [ 'deprecatedVersion' => '1.0' ] );
+			$mockDeprecatedHooks = $this->getMockDeprecatedHooks( [
+				'FooActionComplete' => [ 'deprecatedVersion' => '1.0' ]
+			] );
 			$handler = [
 				'handler' => [
 					'name' => 'Path/To/extension.json-FooActionHandler',
@ -372,6 +382,30 @@ namespace MediaWiki\HookContainer {
 			$this->expectException( UnexpectedValueException::class );
 			$hookContainer->run( 'InvalidReturnHandler' );
 		}
+
+		/**
+		 * @covers       \MediaWiki\HookContainer\HookContainer::emitDeprecationWarnings
+		 */
+		public function testEmitDeprecationWarnings() {
+			$registry = $this->getMockExtensionRegistry(
+				[
+					'handler' => 'FooGlobalFunction',
+					'extensionPath' => 'fake-extension.json'
+				],
+				'FooActionComplete'
+			);
+
+			$hookContainer = $this->newHookContainer(
+				$registry,
+				null,
+				$this->getMockDeprecatedHooks( [
+					'FooActionComplete' => [ 'deprecatedVersion' => '1.35' ]
+				] )
+			);
+
+			$this->expectDeprecation();
+			$hookContainer->emitDeprecationWarnings();
+		}
 	}

 	// Mock class for different types of handler functions