Raise phpstan level from level 3 to level 4 and fixed errors #7096
Conversation
|
Plugin builds for 9301d45 are ready 🛎️!
|
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
Would it be possible to drop a PR comment with each change to show what PHPStan had complained about for each instance? That would help with reviewing. Basically inlining this comment: #4733 (comment).
|
|
||
| // Account for case where registered script is a placeholder for a set of scripts (e.g. jquery). | ||
| if ( isset( $wp_scripts->registered[ $added_handle ] ) && false === $wp_scripts->registered[ $added_handle ]->src ) { | ||
| if ( isset( $wp_scripts->registered[ $added_handle ] ) && empty( $wp_scripts->registered[ $added_handle ]->src ) ) { |
There was a problem hiding this comment.
The value that core is looking for is specifically false. While empty() will have the same effect, it will also match other falsy values. Is this needed because _WP_Dependency::$src is defined as only being a string?
There was a problem hiding this comment.
Below message, I'm getting while running the phpstan.
------ -------------------------------------------------------------------------------------
Line includes/validation/class-amp-validation-callback-wrapper.php
------ -------------------------------------------------------------------------------------
326 Result of && is always false.
326 Strict comparison using === between false and string will always evaluate to false.
------ -------------------------------------------------------------------------------------
Yes, As per documentation in core _WP_Dependency::$src have only a string value.
There was a problem hiding this comment.
Seems like that's something that WordPress core should fix in its phpdoc as well, but I don't see any downside to using empty() instead of checking for false.
dhaval-parekh
force-pushed
the
tech-debt/4733-raise-phpstan-level
branch
from
1e9be9e to
d2b95e7
Compare
dhaval-parekh
force-pushed
the
tech-debt/4733-raise-phpstan-level
branch
from
fd098c1 to
9ad7d75
Compare
westonruter
added
the
Changelogged
Summary
Fixes #4733
Checklist