-
Notifications
You must be signed in to change notification settings - Fork 86
PHPStan level 9 #453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
PHPStan level 9 #453
Conversation
@@ -875,7 +904,7 @@ protected function get_item_list() { | |||
|
|||
if ( isset( $plugin_update_info->requires ) && version_compare( $wp_version, $requires, '>=' ) ) { | |||
$reason = "This update requires WordPress version $plugin_update_info->requires, but the version installed is $wp_version."; | |||
} elseif ( ! isset( $update_info['package'] ) ) { | |||
} elseif ( ! isset( $plugin_update_info->package ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$update_info
does not exist at this point.
Introduced in #451
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ FYI @mrsdizzie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♀️ yes that is a typo since these are similarly named. I should set up PHPStan (!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing that I am working on a unified setup :-)
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
*/ | ||
public function activate( $args = array() ) { | ||
public function activate( $args, $assoc_args = [] ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed $args
from optional to required. Was this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because it is actually required as per the synopsys, and WP-CLI always passes both arguments to commands anyway. I've never seen $args
or $assoc_args
be null actually, so should even change this to ( $args, $assoc_args )
.
src/Theme_Mod_Command.php
Outdated
@@ -232,6 +232,9 @@ public function remove( $args = array(), $assoc_args = array() ) { | |||
* # Set theme mod | |||
* $ wp theme mod set background_color 000000 | |||
* Success: Theme mod background_color set to 000000. | |||
* | |||
* @param string[] $args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different types then for activate()
above. We should probably define and stick to a consistent type hint for command methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -80,6 +84,8 @@ abstract protected function status_single( $args ); | |||
|
|||
abstract protected function install_from_repo( $slug, $assoc_args ); | |||
|
|||
abstract public function activate( $args, $assoc_args = [] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably stick to consistent command signature as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So $assoc_args
is sometimes optional because the class is extended. The method is sometimes called with 1 parameter from within the sub class itself.
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Mostly doc block improvements, but also fixes a regression from #451