From 6091dca5a61917708b9b514db839b62e2ce536cc Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Thu, 16 Apr 2020 16:20:16 +1000 Subject: [PATCH] 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 --- includes/HookContainer/DeprecatedHooks.php | 36 ++++- includes/HookContainer/HookContainer.php | 29 +++- includes/actions/ViewAction.php | 8 + includes/registration/ExtensionProcessor.php | 80 ++++------ .../registration/ExtensionProcessorTest.php | 137 ++++++++---------- .../HookContainer/HookContainerTest.php | 46 +++++- 6 files changed, 198 insertions(+), 138 deletions(-) diff --git a/includes/HookContainer/DeprecatedHooks.php b/includes/HookContainer/DeprecatedHooks.php index 6bc89708fce..8f3fde79d79 100644 --- 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. diff --git a/includes/HookContainer/HookContainer.php b/includes/HookContainer/HookContainer.php index f92ec72ce9d..c63700136ab 100644 --- 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'] + ); + } + } + } + } + } } diff --git a/includes/actions/ViewAction.php b/includes/actions/ViewAction.php index 769ad387137..00aaa7e82a2 100644 --- 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 diff --git a/includes/registration/ExtensionProcessor.php b/includes/registration/ExtensionProcessor.php index 109d65d6e30..ab9a24a73eb 100644 --- a/includes/registration/ExtensionProcessor.php +++ b/includes/registration/ExtensionProcessor.php @@ -1,7 +1,5 @@ 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; } } } diff --git a/tests/phpunit/includes/registration/ExtensionProcessorTest.php b/tests/phpunit/includes/registration/ExtensionProcessorTest.php index 21582d4e383..a547183eaa7 100644 --- 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 */ diff --git a/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php b/tests/phpunit/unit/includes/HookContainer/HookContainerTest.php index 44c646e63c3..f516bff93fd 100644 --- 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