Skip to content
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

Raise phpstan level from level 3 to level 4 and fixed errors #7096

Merged
merged 10 commits into from
May 18, 2022

Conversation

dhaval-parekh
Copy link
Collaborator

@dhaval-parekh dhaval-parekh commented May 11, 2022

Summary

Fixes #4733

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

phpstan.neon.dist Outdated Show resolved Hide resolved
@dhaval-parekh dhaval-parekh self-assigned this May 11, 2022
@dhaval-parekh dhaval-parekh marked this pull request as ready for review May 11, 2022 14:05
@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2022

Plugin builds for 9301d45 are ready 🛎️!

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/Support/SupportData.php Outdated Show resolved Hide resolved
src/PluginSuppression.php Outdated Show resolved Hide resolved
@@ -323,7 +323,7 @@ protected function finalize_scripts( WP_Scripts $wp_scripts, array $before_regis
$handles = [ $added_handle ];

// 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 ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

includes/amp-helper-functions.php Outdated Show resolved Hide resolved
src/ValidationExemption.php Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
@dhaval-parekh dhaval-parekh force-pushed the tech-debt/4733-raise-phpstan-level branch from 1e9be9e to d2b95e7 Compare May 13, 2022 08:25
@dhaval-parekh dhaval-parekh force-pushed the tech-debt/4733-raise-phpstan-level branch from fd098c1 to 9ad7d75 Compare May 18, 2022 08:20
@westonruter westonruter added this to the v2.3 milestone May 18, 2022
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

@westonruter westonruter merged commit e311a10 into develop May 18, 2022
@westonruter westonruter deleted the tech-debt/4733-raise-phpstan-level branch May 18, 2022 23:07
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise PHPStan level for the AmpProject\AmpWP package to 4
2 participants