Skip to content

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Nov 24, 2024

We can't use PHPStan out of the box right now because of the higher PHP version requirement (see wp-cli/wp-cli-tests#204), so I just ran it manually locally, level by level, fixing bugs along the way.

I went up to level ~5 here.

The array returned by `error_get_last` always has this key
The `WP_CLI::error` call above always terminates execution
`$explanation` is never empty at this point
The variable is always set
This method never returns false
The `RequestsLibrary::get_class_name` method never throws
There is an early return above if `self::$logger` isn't set
@swissspidy swissspidy marked this pull request as ready for review November 25, 2024 09:50
@swissspidy swissspidy requested a review from a team as a code owner November 25, 2024 09:50
@@ -277,7 +277,7 @@ private static function get_aliases( $subcommands ) {
/**
* Composite commands can only be known by one name.
*
* @return false
* @return string|false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this seems confusing since all this method does is return false 🧐 I wonder is this tool getting confused and considering the subcommand version of this method which extends CompositeCommand?

/**
* If an alias is set, grant access to it.
* Aliases permit subcommands to be instantiated
* with a secondary identity.
*
* @return string
*/
public function get_alias() {
return $this->alias;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly. I wouldn't call it confused though, because it's definitely an incorrect documentation.

If the parent method returns only false, a child class returning a string is a type mismatch.

@swissspidy swissspidy merged commit 0187f2b into main Nov 26, 2024
43 of 46 checks passed
@swissspidy swissspidy deleted the try/phpstan branch November 26, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants