-
Notifications
You must be signed in to change notification settings - Fork 107
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
Update PHPStan level to 1 #1198
Conversation
cde5aab
to
de2bb64
Compare
@@ -142,11 +142,11 @@ function dominant_color_img_tag_add_dominant_color( $filtered_image, $context, $ | |||
$extra_class = $image_meta['has_transparency'] ? 'has-transparency' : 'not-transparent'; | |||
} | |||
|
|||
if ( ! empty( $data ) ) { |
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.
PHPStan was complaining here:
Variable $data in empty() always exists and is always falsy.
For some reason switching to a simple boolean check fixed the issue. In any case, using empty()
is discouraged per #1091 (comment)
Same goes for the change to $extra_class
below.
|
||
return $hook_suffix; | ||
} | ||
|
||
// @phpstan-ignore-next-line |
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.
As noted in #1188 (comment) this returning of the $hook_suffix
was only being used in tests. This PR eliminates the need for that return value by instead computing the $hook_suffix
in the test, and then seeing if the expected action is added.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -5,6 +5,9 @@ | |||
* @package performance-lab | |||
*/ | |||
|
|||
define( 'TESTS_PLUGIN_DIR', './' ); | |||
define( 'WPINC', 'wp-includes' ); |
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.
I guess WPINC
should be added to https://github.com/szepeviktor/phpstan-wordpress
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.
I know there is no core function to get the value of WPINC but I do not want to encourage the users of my package to use WPINC.
de2bb64
to
09d742d
Compare
@@ -88,11 +88,19 @@ function webp_uploads_generate_additional_image_source( $attachment_id, $image_s | |||
* | |||
* @since 1.1.0 | |||
* | |||
* @param array|null|WP_Error $image Image data {'path'=>string, 'file'=>string, 'width'=>int, 'height'=>int, 'mime-type'=>string} or null or WP_Error. |
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.
Note that width
, height
, and mime-type
are actually not used here. The only keys used are file
and either path
or filesize
.
* filesize: int, | ||
* sources?: array<string, array{ file: string, filesize: int }> | ||
* } $metadata | ||
*/ |
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.
Without this type definition, the isset( $metadata['sources'][ $target_mime ]['file'] )
check below fails PHPStan because wp_get_attachment_metadata()
doesn't normally return a sources
key.
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.
I revisited this when bumping PHPStan to level 3. See #1200.
Summary
See #775.
This fixes the following issues when running
npm run phpstan
:[ERROR] Found 109 errors
Relevant technical choices
Please review individual commits.