Merge "Hard deprecate Action::factory with null argument $action"

RELEASE-NOTES-1.35

@@ -863,6 +863,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 ===

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

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

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() {