-
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 7 #1205
Conversation
I've fixed the issues in
The issues for There are also 431 errors remaining in |
… update/phpstan-level-7 * 'trunk' of https://github.com/WordPress/performance: Add missing sources to return tag of webp_uploads_restore_image Improve consistency of phpdoc in return tags Add missing keys to webp_uploads_update_sources arrays Fix param descriptions Fix strpos() argument order error to fix test_tag_add_adjust_to_image_attributes Move deprecated function to deprecated.php Deprecate unused webp_uploads_get_attachment_sources Add test case for when RGB values are too high Ensure RGB variable is int fix: avoid needless array allocation in rgb to hex conversion
When PHPStan 1.11 is out next week, we can easily ignore a lot of the errors in |
$bytes_enqueued = perflab_aea_get_total_size_bytes_enqueued_scripts(); | ||
if ( false === $enqueued_scripts || false === $bytes_enqueued ) { |
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 could be a big fix, since it is possible that the transient for perflab_aea_get_total_size_bytes_enqueued_scripts()
could be empty but not for perflab_aea_get_total_enqueued_scripts()
.
$bytes_enqueued = perflab_aea_get_total_size_bytes_enqueued_styles(); | ||
if ( false === $enqueued_styles || false === $bytes_enqueued ) { |
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.
Ditto above for how this may fix a bug.
$total_size += $enqueued_style['size']; | ||
if ( is_array( $enqueued_style ) && array_key_exists( 'size', $enqueued_style ) && is_int( $enqueued_style['size'] ) ) { | ||
$total_size += $enqueued_style['size']; | ||
} |
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.
Maybe overly-defensive coding, but in reality a transient could return anything. This ensures we're working with what we expect.
@@ -81,7 +81,7 @@ function embed_optimizer_filter_oembed_html( string $html ): string { | |||
// by preventing links in the hidden iframe from receiving focus. | |||
if ( $html_processor->has_class( 'wp-embedded-content' ) ) { | |||
$style = $html_processor->get_attribute( 'style' ); | |||
if ( $style ) { | |||
if ( is_string( $style ) ) { |
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 case only happens if it had parsed <div style>
where there is no attribute value. Unlikely, but it is a hardening.
if ( $breakpoint <= 1 || PHP_INT_MAX === $breakpoint ) { | ||
if ( ! is_int( $breakpoint ) || $breakpoint < 1 || PHP_INT_MAX === $breakpoint ) { |
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 was a minor logic error that likely never would have been encountered, since no one should attempt to have a breakpoint of 1px.
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. |
if ( ! wp_image_editor_supports( array( 'mime_type' => 'image/webp' ) ) ) { | ||
$this->markTestSkipped( 'Mime type image/webp is not supported.' ); | ||
} |
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.
For some reason this test is failing in PHP 7.2 now with:
There was 1 failure:
1) WebP_Uploads_Helper_Tests::it_should_create_an_image_with_filter_webp_uploads_pre_generate_additional_image_source
Failed asserting that WP_Error Object &0000000004c81e5c000000005c809afd (
'errors' => Array &0 (
'image_mime_type_not_supported' => Array &1 (
0 => 'The provided mime type is not supported.'
)
)
'error_data' => Array &2 ()
'additional_data' => Array &3 ()
) is of type "array".
/var/www/html/wp-content/plugins/performance/tests/plugins/webp-uploads/helper-tests.php:294
So I added this.
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.
Ah, it's because of the additional validation I added. Fixed in d8f886b.
281e94f
to
5f6c663
Compare
…dditional_image_source
5f6c663
to
d8f886b
Compare
'filesize' => $filesize, | ||
); | ||
} | ||
} |
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 improved/hardened validation logic resulted in the test change tests/plugins/webp-uploads/helper-tests.php
wherein the filtered value was pointing to an invalid path
which was not resulting in an incorrect overridden value. Now that there is validation for the filtered value, the value will only be used if it is valid.
overall looks good, left a couple of questions. Very interesting to see all the hardening and sanity checks added here, seems useful if tedious. |
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
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.
Nifty!
See #775.
This addresses the following PHPStan issues:
[ERROR] Found 512 errors
Please review individual commits for changes.
I've ignored most of the errors in
tests
since they are overly strict and likely aren't worth fixing. The only other ignored errors are indominant-color-images
where PHP 8 changed the function signature fromresource
toGdImage
.