Fix and enable FunctionComment.MissingReturn PHPCS rule

It looks like this is worth it. There are not that many issues found, which implies that almost all of this codebase was already following this rule. I tried to apply the most minimal fixes as well as reduce duplication by utilizing the @inheritdoc feature. It effectively marks a method as "intentionally undocumented". Change-Id: I394ad1fc9b918b0b7896bb47d60723587e174f83
wikimedia · Oct 17, 2023 · d461c15 · d461c15
1 parent 9edb1d1
commit d461c15
Show file tree

Hide file tree

Showing 25 changed files with 47 additions and 123 deletions.
diff --git a/.phpcs.xml b/.phpcs.xml
@@ -5,7 +5,6 @@
 		<exclude name="MediaWiki.Commenting.FunctionComment.MissingDocumentationProtected" />
 		<exclude name="MediaWiki.Commenting.FunctionComment.MissingDocumentationPublic" />
 		<exclude name="MediaWiki.Commenting.FunctionComment.MissingParamTag" />
-		<exclude name="MediaWiki.Commenting.FunctionComment.MissingReturn" />
 		<!-- Note: NotPunctuationReturnType can be enabled when the sniff accepts array shapes -->
 		<exclude name="MediaWiki.Commenting.FunctionComment.NotPunctuationReturnType" />
 		<!-- Note: ParamNameNoMatch is probably a great candidate for the next round of fixes -->

diff --git a/client/includes/MaintenanceShellStart.php b/client/includes/MaintenanceShellStart.php
@@ -4,6 +4,7 @@
 use Wikibase\Client\WikibaseClient;
 
 // phpcs:disable Squiz.Functions.GlobalFunction.Found
+// phpcs:disable MediaWiki.Commenting.FunctionComment.MissingReturn
 // phpcs:disable MediaWiki.NamingConventions.PrefixedGlobalFunctions.wfPrefix
 
 if ( !function_exists( 'mws' ) ) {

diff --git a/client/tests/phpunit/unit/includes/Hooks/SkinAfterBottomScriptsHandlerTest.php b/client/tests/phpunit/unit/includes/Hooks/SkinAfterBottomScriptsHandlerTest.php
@@ -134,6 +134,7 @@ private function mockTitle( $baseURL, $titleText ) {
 
 	/**
 	 * @param string|null $timestamp
+	 * @return RevisionLookup
 	 */
 	private function createMockRevisionLookup( $timestamp ) {
 		$revisionRecord = $this->createMock( RevisionRecord::class );

diff --git a/lib/packages/wikibase/data-model/tests/unit/NewItem.php b/lib/packages/wikibase/data-model/tests/unit/NewItem.php
@@ -31,7 +31,7 @@ class NewItem {
 	private $descriptions = [];
 
 	/**
-	 * @var array[] Indexed by language on the first level
+	 * @var array<string,string[]> Indexed by language code on the first level
 	 */
 	private $aliases = [];
 
@@ -86,15 +86,15 @@ public function build() {
 	}
 
 	/**
-	 * @see andId
+	 * @param ItemId|string $itemId
+	 * @return self
 	 */
 	public static function withId( $itemId ) {
 		return ( new self() )->andId( $itemId );
 	}
 
 	/**
 	 * @param ItemId|string $itemId
-	 *
 	 * @return self
 	 */
 	public function andId( $itemId ) {
@@ -107,7 +107,9 @@ public function andId( $itemId ) {
 	}
 
 	/**
-	 * @see andLabel
+	 * @param string $languageCode
+	 * @param string $label
+	 * @return self
 	 */
 	public static function withLabel( $languageCode, $label ) {
 		return ( new self() )->andLabel( $languageCode, $label );
@@ -116,7 +118,6 @@ public static function withLabel( $languageCode, $label ) {
 	/**
 	 * @param string $languageCode
 	 * @param string $label
-	 *
 	 * @return self
 	 */
 	public function andLabel( $languageCode, $label ) {
@@ -126,7 +127,9 @@ public function andLabel( $languageCode, $label ) {
 	}
 
 	/**
-	 * @see andDescription
+	 * @param string $languageCode
+	 * @param string $description
+	 * @return self
 	 */
 	public static function withDescription( $languageCode, $description ) {
 		return ( new self() )->andDescription( $languageCode, $description );
@@ -135,7 +138,6 @@ public static function withDescription( $languageCode, $description ) {
 	/**
 	 * @param string $languageCode
 	 * @param string $description
-	 *
 	 * @return self
 	 */
 	public function andDescription( $languageCode, $description ) {
@@ -145,7 +147,9 @@ public function andDescription( $languageCode, $description ) {
 	}
 
 	/**
-	 * @see andAliases
+	 * @param string $languageCode
+	 * @param string[]|string $aliases
+	 * @return self
 	 */
 	public static function withAliases( $languageCode, $aliases ) {
 		return ( new self() )->andAliases( $languageCode, $aliases );
@@ -154,7 +158,6 @@ public static function withAliases( $languageCode, $aliases ) {
 	/**
 	 * @param string $languageCode
 	 * @param string[]|string $aliases
-	 *
 	 * @return self
 	 */
 	public function andAliases( $languageCode, $aliases ) {
@@ -164,7 +167,11 @@ public function andAliases( $languageCode, $aliases ) {
 	}
 
 	/**
-	 * @see andSiteLink
+	 * @param string $siteId
+	 * @param string $pageName
+	 * @param ItemId[]|string[]|ItemId|string|null $badges Zero or more item ID references as either
+	 *  strings or ItemId objects. Can be an array or a single value.
+	 * @return self
 	 */
 	public static function withSiteLink( $siteId, $pageName, $badges = null ) {
 		return ( new self() )->andSiteLink( $siteId, $pageName, $badges );
@@ -175,7 +182,6 @@ public static function withSiteLink( $siteId, $pageName, $badges = null ) {
 	 * @param string $pageName
 	 * @param ItemId[]|string[]|ItemId|string|null $badges Zero or more item ID references as either
 	 *  strings or ItemId objects. Can be an array or a single value.
-	 *
 	 * @return self
 	 */
 	public function andSiteLink( $siteId, $pageName, $badges = null ) {
@@ -190,15 +196,15 @@ public function andSiteLink( $siteId, $pageName, $badges = null ) {
 	}
 
 	/**
-	 * @see andStatement
+	 * @param NewStatement|Statement|Snak $statement
+	 * @return self
 	 */
 	public static function withStatement( $statement ) {
 		return ( new self() )->andStatement( $statement );
 	}
 
 	/**
 	 * @param NewStatement|Statement|Snak $statement
-	 *
 	 * @return self
 	 */
 	public function andStatement( $statement ) {

diff --git a/lib/tests/phpunit/Formatters/ItemPropertyIdHtmlLinkFormatterTest.php b/lib/tests/phpunit/Formatters/ItemPropertyIdHtmlLinkFormatterTest.php
@@ -538,6 +538,8 @@ private function givenItemDoesNotExist( $itemId ) {
 
 	/**
 	 * @param string $itemId
+	 * phpcs:ignore MediaWiki.Commenting.FunctionComment.ObjectTypeHintReturn
+	 * @return object
 	 */
 	private function givenItemExists( $itemId ) {
 		$title = $this->createMock( Title::class );

diff --git a/repo/includes/ChangeOp/ChangeOpAliases.php b/repo/includes/ChangeOp/ChangeOpAliases.php
@@ -95,15 +95,7 @@ private function updateAliases( AliasGroupList $aliases ) {
 		$aliases->setAliasesForLanguage( $this->languageCode, $newAliases );
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof AliasesProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a AliasesProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpDescription.php b/repo/includes/ChangeOp/ChangeOpDescription.php
@@ -95,15 +95,7 @@ private function buildResult( EntityId $entityId = null, Term $oldDescription =
 		return new ChangeOpDescriptionResult( $entityId, $this->languageCode, $oldDescriptionText, $newDescriptionText, $isEntityChanged );
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof DescriptionsProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a DescriptionsProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpLabel.php b/repo/includes/ChangeOp/ChangeOpLabel.php
@@ -94,14 +94,7 @@ private function buildResult( EntityId $entityId = null, Term $oldLabel = null,
 		return new ChangeOpLabelResult( $entityId, $this->languageCode, $oldLabelText, $newLabelText, $isEntityChanged );
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof LabelsProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a LabelsProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpMainSnak.php b/repo/includes/ChangeOp/ChangeOpMainSnak.php
@@ -77,15 +77,10 @@ public function getStatementGuid() {
 	}
 
 	/**
-	 * @see ChangeOp::apply()
 	 * - a new claim with $snak as mainsnak gets added when $claimGuid is empty and $snak is set
 	 * - the claim's mainsnak gets set to $snak when $claimGuid and $snak are set
 	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
+	 * @inheritDoc
 	 */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {

diff --git a/repo/includes/ChangeOp/ChangeOpQualifier.php b/repo/includes/ChangeOp/ChangeOpQualifier.php
@@ -66,15 +66,10 @@ public function __construct( $statementGuid, Snak $snak, $snakHash, SnakValidato
 	}
 
 	/**
-	 * @see ChangeOp::apply()
 	 * - a new qualifier gets added when $snakHash is empty and $snak is set
 	 * - the qualifier gets set to $snak when $snakHash and $snak are set
 	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
+	 * @inheritDoc
 	 */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {

diff --git a/repo/includes/ChangeOp/ChangeOpQualifierRemove.php b/repo/includes/ChangeOp/ChangeOpQualifierRemove.php
@@ -49,15 +49,7 @@ public function __construct( $statementGuid, $snakHash ) {
 		$this->snakHash = $snakHash;
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a StatementListProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpReference.php b/repo/includes/ChangeOp/ChangeOpReference.php
@@ -84,15 +84,10 @@ public function __construct(
 	}
 
 	/**
-	 * @see ChangeOp::apply()
 	 * - a new reference gets added when $referenceHash is empty and $reference is set
 	 * - the reference gets set to $reference when $referenceHash and $reference are set
 	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
+	 * @inheritDoc
 	 */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {

diff --git a/repo/includes/ChangeOp/ChangeOpReferenceRemove.php b/repo/includes/ChangeOp/ChangeOpReferenceRemove.php
@@ -49,15 +49,7 @@ public function __construct( $statementGuid, $referenceHash ) {
 		$this->referenceHash = $referenceHash;
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a StatementListProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpRemoveStatement.php b/repo/includes/ChangeOp/ChangeOpRemoveStatement.php
@@ -45,15 +45,7 @@ public function __construct( $guid ) {
 		$this->guid = $guid;
 	}
 
-	/**
-	 * @see ChangeOp::apply
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a StatementListProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpStatement.php b/repo/includes/ChangeOp/ChangeOpStatement.php
@@ -86,15 +86,7 @@ public function __construct(
 		$this->index = $index;
 	}
 
-	/**
-	 * @see ChangeOp::apply
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a StatementListProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOpStatementRank.php b/repo/includes/ChangeOp/ChangeOpStatementRank.php
@@ -49,15 +49,7 @@ public function __construct( $statementGuid, $rank ) {
 		$this->rank = $rank;
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws InvalidArgumentException
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		if ( !( $entity instanceof StatementListProvider ) ) {
 			throw new InvalidArgumentException( '$entity must be a StatementListProvider' );

diff --git a/repo/includes/ChangeOp/ChangeOps.php b/repo/includes/ChangeOp/ChangeOps.php
@@ -62,15 +62,7 @@ public function getChangeOps() {
 		return $this->changeOps;
 	}
 
-	/**
-	 * @see ChangeOp::apply()
-	 * Applies all changes to the given entity
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary
-	 *
-	 * @throws ChangeOpException
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		$changeOpsResults = [];
 

diff --git a/repo/includes/ChangeOp/ChangeOpsMerge.php b/repo/includes/ChangeOp/ChangeOpsMerge.php
@@ -137,6 +137,7 @@ private function getSiteLinkChangeOpFactory() {
 
 	/**
 	 * @throws ChangeOpException
+	 * @return ChangeOpResult
 	 */
 	public function apply() {
 		// NOTE: we don't want to validate the ChangeOps individually, since they represent

diff --git a/repo/includes/ChangeOp/NullChangeOp.php b/repo/includes/ChangeOp/NullChangeOp.php
@@ -22,12 +22,7 @@ public function validate( EntityDocument $entity ) {
 		return Result::newSuccess();
 	}
 
-	/**
-	 * @see ChangeOp::apply
-	 *
-	 * @param EntityDocument $entity
-	 * @param Summary|null $summary Unused
-	 */
+	/** @inheritDoc */
 	public function apply( EntityDocument $entity, Summary $summary = null ) {
 		// no op
 

diff --git a/repo/includes/MaintenanceShellStart.php b/repo/includes/MaintenanceShellStart.php
@@ -4,6 +4,7 @@
 use Wikibase\Repo\WikibaseRepo;
 
 // phpcs:disable Squiz.Functions.GlobalFunction.Found
+// phpcs:disable MediaWiki.Commenting.FunctionComment.MissingReturn
 // phpcs:disable MediaWiki.NamingConventions.PrefixedGlobalFunctions.wfPrefix
 
 if ( !function_exists( 'mws' ) ) {

diff --git a/repo/includes/RepoHooks.php b/repo/includes/RepoHooks.php
@@ -159,8 +159,6 @@ public static function onSetupAfterCache() {
 				return $entityContentFactory->getContentHandlerForType( $entityType );
 			};
 		}
-
-		return true;
 	}
 
 	/**