From 385d1910250980bee72eaacdffa85fc82bb9b3ae Mon Sep 17 00:00:00 2001 From: ArtBaltai Date: Wed, 1 Apr 2020 07:09:36 +0300 Subject: [PATCH] Hard deprecate Action::factory with null argument $action Bug: T248392 Change-Id: I68293a716ec90e5fd944c45d6ccd432ad3b82beb --- RELEASE-NOTES-1.35 | 1 + includes/MediaWiki.php | 2 +- includes/actions/Action.php | 14 ++++-- tests/phpunit/includes/actions/ActionTest.php | 46 +++++++++++++------ 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/RELEASE-NOTES-1.35 b/RELEASE-NOTES-1.35 index 15b9f1913ab..e9a7a4fd24e 100644 --- a/RELEASE-NOTES-1.35 +++ b/RELEASE-NOTES-1.35 @@ -855,6 +855,7 @@ because of Phabricator reports. * Page interface was deprecated. Use Article or WikiPage instead. * wfGetScriptUrl() was deprecated. The script URL should be configured rather than detected. wfScript() can be used to get a configured script URL. +* Action::factory() with null $action argument is hard deprecated * … === Other changes in 1.35 === diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index 5cb69b74bc7..d03de28d62a 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -152,7 +152,7 @@ class MediaWiki { * * @return string Action */ - public function getAction() { + public function getAction() : string { if ( $this->action === null ) { $this->action = Action::getActionName( $this->context ); } diff --git a/includes/actions/Action.php b/includes/actions/Action.php index ab1dc58497c..0e8f6d46f1a 100644 --- a/includes/actions/Action.php +++ b/includes/actions/Action.php @@ -75,7 +75,10 @@ abstract class Action implements MessageLocalizer { * @param array $overrides * @return bool|null|string|callable|Action */ - final private static function getClass( $action, array $overrides ) { + final private static function getClass( + string $action, + array $overrides + ) { global $wgActions; $action = strtolower( $action ); @@ -98,8 +101,7 @@ abstract class Action implements MessageLocalizer { * Get an appropriate Action subclass for the given action * @since 1.17 * - * - * @param string|null $action + * @param string|null $action Null is hard-deprecated since 1.35 * @param Article|WikiPage|Page $article * Calling with anything other than Article is deprecated since 1.35 * @param IContextSource|null $context @@ -111,6 +113,10 @@ abstract class Action implements MessageLocalizer { Page $article, IContextSource $context = null ) { + if ( !is_string( $action ) ) { + wfDeprecated( __METHOD__ . ' with null $action', '1.35' ); + return null; + } $classOrCallable = self::getClass( $action, $article->getActionOverrides() ); if ( is_string( $classOrCallable ) ) { if ( !class_exists( $classOrCallable ) ) { @@ -187,7 +193,7 @@ abstract class Action implements MessageLocalizer { * @param string $name Name of an action * @return bool */ - final public static function exists( $name ) { + final public static function exists( string $name ) : bool { return self::getClass( $name, [] ) !== null; } diff --git a/tests/phpunit/includes/actions/ActionTest.php b/tests/phpunit/includes/actions/ActionTest.php index d41c65e4eeb..bc9449ff9c8 100644 --- a/tests/phpunit/includes/actions/ActionTest.php +++ b/tests/phpunit/includes/actions/ActionTest.php @@ -63,11 +63,10 @@ class ActionTest extends MediaWikiTestCase { [ 'DUMMY', 'DummyAction' ], [ 'STRING', 'NamedDummyAction' ], - // Null and non-existing values + // non-existing values [ 'null', null ], [ 'undeclared', null ], [ '', null ], - [ false, null ], ]; } @@ -76,7 +75,7 @@ class ActionTest extends MediaWikiTestCase { * @param string $requestedAction * @param string|null $expected */ - public function testActionExists( $requestedAction, $expected ) { + public function testActionExists( string $requestedAction, $expected ) { $exists = Action::exists( $requestedAction ); $this->assertSame( $expected !== null, $exists ); @@ -101,6 +100,25 @@ class ActionTest extends MediaWikiTestCase { $this->assertEquals( $expected ?: 'nosuchaction', $actionName ); } + public function provideGetActionNameNotPossible() { + return [ + 'null' => [ null, 'view' ], + 'false' => [ false, 'nosuchaction' ], + ]; + } + + /** + * @dataProvider provideGetActionNameNotPossible + * @param mixed $requestedAction + * @param string $expected + */ + public function testGetActionNameNotPossible( $requestedAction, string $expected ) { + $actionName = Action::getActionName( + $this->getContext( $requestedAction ) + ); + $this->assertEquals( $expected, $actionName ); + } + public function testGetActionName_editredlinkWorkaround() { // See https://phabricator.wikimedia.org/T22966 $context = $this->getContext( 'editredlink' ); @@ -136,11 +154,13 @@ class ActionTest extends MediaWikiTestCase { } /** + * @covers \Action::factory + * * @dataProvider actionProvider * @param string $requestedAction * @param string|null $expected */ - public function testActionFactory( $requestedAction, $expected ) { + public function testActionFactory( string $requestedAction, $expected ) { $context = $this->getContext(); $action = Action::factory( $requestedAction, $context->getWikiPage(), $context ); @@ -151,12 +171,6 @@ class ActionTest extends MediaWikiTestCase { } } - public function testNull_doesNotExist() { - $exists = Action::exists( null ); - - $this->assertFalse( $exists ); - } - public function testNull_defaultsToView() { $context = $this->getContext( null ); $actionName = Action::getActionName( $context ); @@ -164,11 +178,13 @@ class ActionTest extends MediaWikiTestCase { $this->assertEquals( 'view', $actionName ); } - public function testNull_canNotBeInstantiated() { - $page = $this->getPage(); - $action = Action::factory( null, $page ); - - $this->assertNull( $action ); + /** + * @covers \Action::factory + */ + public function testActionFactory_withNull_expectNull() { + $this->hideDeprecated( 'Action::factory with null $action' ); + $result = Action::factory( null, $this->getPage() ); + $this->assertNull( $result ); } public function testDisabledAction_exists() {