-
Notifications
You must be signed in to change notification settings - Fork 98
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 to 1.11.5 #1318
Update PHPStan to 1.11.5 #1318
Conversation
fea003c
to
60dc6f4
Compare
@@ -32,11 +32,11 @@ public function get_dominant_color() { | |||
} | |||
// The logic here is resize the image to 1x1 pixel, then get the color of that pixel. | |||
$shorted_image = imagecreatetruecolor( 1, 1 ); | |||
$image_width = imagesx( $this->image ); | |||
$image_height = imagesy( $this->image ); | |||
if ( false === $shorted_image || false === $image_width || false === $image_height ) { |
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.
In PhpStorm's stubs, it defines:
/**
* Create a new true color image
* @link https://php.net/manual/en/function.imagecreatetruecolor.php
* @param int $width <p>
* Image width.
* </p>
* @param int $height <p>
* Image height.
* </p>
* @return resource|GdImage|false an image resource identifier on success, false on errors.
*/
#[Pure]
function imagecreatetruecolor(int $width, int $height): GdImage|false {}
/**
* Get image width
* @link https://php.net/manual/en/function.imagesx.php
* @param resource|GdImage $image
* @return int|false Return the width of the image or false on
* errors.
*/
#[Pure]
function imagesx(GdImage $image): int {}
/**
* Get image height
* @link https://php.net/manual/en/function.imagesy.php
* @param resource|GdImage $image
* @return int|false Return the height of the image or false on
* errors.
*/
#[Pure]
function imagesy(GdImage $image): int {}
All three have phpdoc that say they can return false
, but only imagecreatetruecolor()
has the PHP type hint which says it can return false
, and this is confirmed in the PHP docs: https://www.php.net/manual/en/function.imagecreatetruecolor.php
In contrast, the PHP docs for the other functions say nothing about them returning false:
https://www.php.net/manual/en/function.imagesx.php
https://www.php.net/manual/en/function.imagesy.php
So it seems PhpStorm's docs are incorrect.
if ( ! is_array( $rgba ) ) { | ||
return new WP_Error( 'unable_to_obtain_rgba_via_imagecolorsforindex' ); | ||
} |
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.
PHP docs say that this function only returns an array: https://www.php.net/manual/en/function.imagecolorsforindex.php
But PhpStorm's stubs say it can return false
:
/**
* Get the colors for an index
* @link https://php.net/manual/en/function.imagecolorsforindex.php
* @param resource|GdImage $image
* @param int $color <p>
* The color index.
* </p>
* @return array|false an associative array with red, green, blue and alpha keys that
* contain the appropriate values for the specified color index or <b>FALSE</b> on failure
*/
#[Pure]
#[LanguageLevelTypeAware(['8.0' => 'array'], default: 'array|false')]
#[ArrayShape(["red" => "int", "green" => "int", "blue" => "int", "alpha" => "int"])]
function imagecolorsforindex(GdImage $image, int $color) {}
Again, it seems PhpStorm is wrong.
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.
According to the changelog:
imagecolorsforindex() now throws a ValueError exception if color is out of range; previously, false was returned instead.
I wonder what versions of GD we need to support?
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, then we should probably wrap this in a try/catch block to account for this.
foreach ( $backup_sizes as $size => $properties ) { | ||
$size_name = str_replace( '-orig', '', $size ); | ||
|
||
if ( 'full-orig' === $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.
I have no idea why PHPStan is saying that this is an error:
Strict comparison using === between 'full-orig' and NEVER will always evaluate to false.
In any case, it seems better to just unset the array key rather than to check if with each iteration.
|
||
$metadata = wp_get_attachment_metadata( $attachment_id ); |
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 appears to be dead code, perhaps code which was going to have assertions added to it, but they never were written.
@@ -136,7 +130,7 @@ public function test_it_should_prevent_to_back_up_the_sources_when_the_sources_a | |||
$backup_sizes = get_post_meta( $attachment_id, '_wp_attachment_backup_sizes', true ); | |||
$this->assertIsArray( $backup_sizes ); | |||
|
|||
foreach ( $backup_sizes as $size_name => $properties ) { |
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.
Removing unused variable.
@@ -281,7 +275,7 @@ static function () { | |||
foreach ( $metadata['sizes'] as $size_name => $properties ) { | |||
$this->assertImageNotHasSizeSource( $attachment_id, $size_name, 'image/jpeg' ); | |||
$this->assertImageHasSizeSource( $attachment_id, $size_name, 'image/webp' ); | |||
foreach ( $properties['sources'] as $mime_type => $values ) { |
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.
Removing unused variable.
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. |
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.
Looks good to me.
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.
The changes look good to me, and I pre-approve the PR. There is one piece of feedback that needs to be addressed: #1318 (comment)
@westonruter @joemcgill I recommend that we stick to a specific PHPStan version for now, as it blocks the progress of other PRs. When I worked on #1252, I encountered similar errors, and it didn't allow me to push changes without resolving these PHPStan errors. Sticking to the current version will prevent blocking other PRs and we can plan to update eventually when a new version comes up, ensuring a smooth workflow. |
@mukeshpanchal27 I don't know if I agree. When would we upgrade then? Resolving PHPStan errors should in general just make the code better. In your PR #1252 you may have gotten PHPStan errors if you switched to this branch, ran |
The following PHPStan failures are occurring in PHPStan 1.11.5: