Add PHPStan strict rules (except for empty.notAllowed) #1241

westonruter · 2024-05-23T22:45:18Z

Note: This does not fix the empty.notAllowed issues since they are more complicated and deserve a separate PR.

Just adding phpstan-strict-rules results in the following 863 errors in the codebase:

    712 staticMethod.dynamicCall
     60 empty.notAllowed
     36 booleanNot.exprNotBoolean
     21 if.condNotBoolean
     11 booleanAnd.leftNotBoolean
      5 cast.useless
      4 booleanOr.rightNotBoolean
      4 arrayFilter.strict
      3 method.childReturnType
      2 ternary.condNotBoolean
      2 booleanAnd.rightNotBoolean
      1 variable.implicitArray
      1 plus.leftNonNumeric
      1 elseif.condNotBoolean

This report was generated via:

npm run phpstan -- -- -v | grep 🪪 | cut -c16- | sed 's/ *$//' | sort | uniq -c | sort -nr

When ignoring staticMethod.dynamicCall, booleanNot.exprNotBoolean, and if.condNotBoolean in tests, this goes down to just 144 errors:

     60 empty.notAllowed
     34 booleanNot.exprNotBoolean
     16 if.condNotBoolean
     11 booleanAnd.leftNotBoolean
      5 cast.useless
      4 booleanOr.rightNotBoolean
      4 arrayFilter.strict
      3 method.childReturnType
      2 ternary.condNotBoolean
      2 booleanAnd.rightNotBoolean
      1 variable.implicitArray
      1 plus.leftNonNumeric
      1 elseif.condNotBoolean

The following remain to be addressed:

…ndNotBoolean for tests

westonruter · 2024-06-13T10:05:17Z

The one last PHPStan error remaining is empty.notAllowed. There are 60 instances of this.

westonruter · 2024-06-13T10:06:44Z

phpstan.neon.dist

@@ -57,3 +59,6 @@ parameters:
 		-
 			identifier: cast.string
 			path: */tests/*
+		-
+			identifier: staticMethod.dynamicCall


I'm not sure why this issue is being reported for assertion method calls in tests.

github-actions · 2024-06-13T10:13:44Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

joemcgill

This looks good to me. I've added some suggestions, but none blocking so I'm pre-approving. Feel free to apply or ignore.

plugins/embed-optimizer/hooks.php

joemcgill · 2024-07-11T16:56:44Z

plugins/embed-optimizer/hooks.php

@@ -63,7 +63,7 @@ function embed_optimizer_filter_oembed_html( string $html ): string {
 	if ( 1 === $script_count && ! $has_inline_script && $html_processor->has_bookmark( 'script' ) ) {
 		add_action( 'wp_footer', 'embed_optimizer_lazy_load_scripts' );
 		if ( $html_processor->seek( 'script' ) ) {
-			if ( $html_processor->get_attribute( 'type' ) ) {
+			if ( is_string( $html_processor->get_attribute( 'type' ) ) ) {


Same as above.

Suggested change

if ( is_string( $html_processor->get_attribute( 'type' ) ) ) {

if ( null !== $html_processor->get_attribute( 'type' ) ) ) {

In this case, I think the code should stay as-is. Since get_attribute can return true in the case of a boolean attribute, we wouldn't want to try passing a true to set_attrbute(). I believe PHPStan would complain about that anyway.

joemcgill · 2024-07-11T17:03:43Z

plugins/performance-lab/includes/admin/load.php

@@ -465,7 +465,7 @@ function perflab_get_plugin_settings_url( string $plugin_slug ): ?string {
 		return null;
 	}
 	$href = $p->get_attribute( 'href' );
-	if ( $href && is_string( $href ) ) {
+	if ( is_string( $href ) && '' !== $href ) {


I could go either way here, but for consistency...

Suggested change

if ( is_string( $href ) && '' !== $href ) {

if ( null !== $href && '' !== $href ) {

This should stay as is because if <a href> is encountered, then $href here would be true which would be a type error since the function returns a string or null.

joemcgill · 2024-07-11T17:06:53Z

plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php

@@ -67,7 +67,7 @@ public function register_metric( string $metric_slug, array $args ): void {
 			return;
 		}

-		if ( did_action( 'perflab_server_timing_send_header' ) && ! doing_action( 'perflab_server_timing_send_header' ) ) {
+		if ( 0 !== did_action( 'perflab_server_timing_send_header' ) && ! doing_action( 'perflab_server_timing_send_header' ) ) {


This is such a common pattern in WP that I wouldn't even think of needing to specify, but makes sense.

joemcgill · 2024-07-11T17:09:03Z

plugins/performance-lab/includes/site-health/audit-enqueued-assets/helper.php

+	if ( '' === $resource_url ) {
 		return '';


Could just return the value early in this case.

Suggested change

if ( '' === $resource_url ) {

return '';

if ( '' === $resource_url ) {

return $resource_url;

Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>

swissspidy

LGTM, pending the merge conflict resolution

… add/phpstan-strict-rules * 'trunk' of https://github.com/WordPress/performance: (206 commits) fix(webp-uploads): prevent picture element when JPEG output disabled Prepare 0.4.1 release of Optimization Detective Update attribute order in Image Prioritizer test after eliminating excessive seek() calls Account for visitors calling next_token() not just seek() Update attribute order in embed-optimizer test due to not seeking Add test to ensure tag visitation does not exceed seek limit Fix attribute order in tests now that seeking is not happening Add missing bookmark name to warning message Only seek back to the current tag if a tag visitor seeked Migrate commander for v12 Upgrade commander to 12.1.0 Bump yoast/phpunit-polyfills from 1.1.1 to 2.0.1 Bump @wordpress/scripts from 26.19.0 to 28.3.0 Bump @wordpress/env from 9.6.0 to 10.3.0 Migrate husky Update Octokit to use dynamic import Bump husky from 8.0.3 to 9.1.0 Add labels for Dependabot PRs Bump web-vitals from 3.5.0 to 4.2.1 Bump phpstan/extension-installer from 1.3.1 to 1.4.1 ...

… add/phpstan-strict-rules * 'trunk' of https://github.com/WordPress/performance: Fix OD test case for WP 6.7-alpha

mukeshpanchal27 · 2024-07-23T04:04:58Z

plugins/webp-uploads/hooks.php

+		if (
+			! is_string( $class_name )
+			||
+			1 !== preg_match( '/(?:^|\s)wp-image-([1-9]\d*)(?:\s|$)/i', $class_name, $matches )


Suggested change

1 !== preg_match( '/(?:^|\s)wp-image-([1-9]\d*)(?:\s|$)/i', $class_name, $matches )

1 !== (int) preg_match( '/(?:^|\s)wp-image-([1-9]\d*)(?:\s|$)/i', $class_name, $matches )

Do we needs to cast int here?

No, because it can either be an int or false, and we're checking to see if it is not identical to the value.

westonruter · 2024-07-23T04:28:44Z

plugins/webp-uploads/hooks.php

@@ -534,22 +534,26 @@ function webp_uploads_update_image_references( $content ): string {

 	// This content does not have any tag on it, move forward.
 	// TODO: Eventually this should use the HTML API to parse out the image tags and then update them.
-	if ( ! preg_match_all( '/<(img)\s[^>]+>/', $content, $img_tags, PREG_SET_ORDER ) ) {
+	if ( 0 === (int) preg_match_all( '/<(img)\s[^>]+>/', $content, $img_tags, PREG_SET_ORDER ) ) {


The use of (int) isn't needed here either...

Well, actually it is. Because it's preg_match_all() so we don't know the number of image tags being matched. Otherwise, it could be 1 === .... So by casting the return value to (int), it will convert both 0 and false to 0. Alternatively, this could be:

Suggested change

if ( 0 === (int) preg_match_all( '/<(img)\s[^>]+>/', $content, $img_tags, PREG_SET_ORDER ) ) {

if ( false === (bool) preg_match_all( '/<(img)\s[^>]+>/', $content, $img_tags, PREG_SET_ORDER ) ) {

But false indicates the error state. The integer return value is the normal case, the number of matches.

plugins/webp-uploads/hooks.php

tests/bootstrap.php

mukeshpanchal27

Thanks @westonruter, LGTM!

mukeshpanchal27 · 2024-07-23T04:58:16Z

@westonruter Could you please merge as it not allow me. Shows that Merging can be performed automatically with 1 approving review.

westonruter added this to the performance-lab 3.2.0 milestone May 23, 2024

westonruter force-pushed the add/phpstan-strict-rules branch from 031d197 to 220bdfa Compare May 23, 2024 22:45

westonruter added the [Type] Enhancement A suggestion for improvement of an existing feature label May 23, 2024

westonruter force-pushed the add/phpstan-strict-rules branch from 1f74e35 to 0dc2c63 Compare May 23, 2024 23:11

westonruter mentioned this pull request May 24, 2024

Add picture element support #73

Merged

westonruter modified the milestones: performance-lab 3.2.0, performance-lab n.e.x.t Jun 4, 2024

westonruter added 9 commits June 13, 2024 10:30

Add phpstan-strict-rules

94f4f22

Ignore staticMethod.dynamicCall, booleanNot.exprNotBoolean, and if.co…

211d192

…ndNotBoolean for tests

Fix cast.useless

f2c5120

Fix method.childReturnType

0c5c42b

Fix plus.leftNonNumeric

66dd4d6

Disable strictArrayFilter rule

db11733

Fix variable.implicitArray

165b15b

Fix instance of booleanAnd.leftNotBoolean

d3e0f22

Update phpstan to 1.11.4

db82f1e

westonruter force-pushed the add/phpstan-strict-rules branch from 00f04f7 to db82f1e Compare June 13, 2024 08:35

westonruter added 6 commits June 13, 2024 10:57

Fix interface.nameCase

7de2d96

Fix booleanNot.exprNotBoolean

87ecaad

Fix ternary.condNotBoolean

44d8375

Fix booleanOr.rightNotBoolean

94884c5

Fix booleanAnd.leftNotBoolean

5b5cd68

Fix if.condNotBoolean

dad5312

westonruter commented Jun 13, 2024

View reviewed changes

Ignore empty.notAllowed temporarily

228035f

westonruter changed the title ~~Add PHPStan strict rules~~ Add PHPStan strict rules (except for empty.notAllowed) Jun 13, 2024

westonruter marked this pull request as ready for review June 13, 2024 10:13

westonruter requested review from felixarntz and adamsilverstein as code owners June 13, 2024 10:13

westonruter requested review from pbearne and spacedmonkey as code owners June 13, 2024 10:13

westonruter requested a review from mukeshpanchal27 June 13, 2024 10:13

westonruter requested a review from swissspidy June 13, 2024 10:14

westonruter mentioned this pull request Jul 1, 2024

Upgrade PHPStan to 1.11.6 #1325

Merged

westonruter modified the milestones: performance-lab 3.3.0, performance-lab n.e.x.t Jul 9, 2024

joemcgill approved these changes Jul 11, 2024

View reviewed changes

Treat a boolean src attribute as if it is an inline script

831105c

Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>

westonruter mentioned this pull request Jul 17, 2024

Fix setting sizes attribute on PICTURE > SOURCE elements #1354

Merged

swissspidy approved these changes Jul 22, 2024

View reviewed changes

westonruter added 4 commits July 22, 2024 13:43

Fix remaining strict rule violations

0413c60

Merge branch 'trunk' of https://github.com/WordPress/performance into…

350ddbf

… add/phpstan-strict-rules * 'trunk' of https://github.com/WordPress/performance: Fix OD test case for WP 6.7-alpha

Fix and improve conditions in auto_sizes_filter_image_tag and add typing

d4af3a6

mukeshpanchal27 reviewed Jul 23, 2024

View reviewed changes

westonruter commented Jul 23, 2024

View reviewed changes

plugins/webp-uploads/hooks.php Outdated Show resolved Hide resolved

Remove unnecessary casting to int

8e90f62

westonruter commented Jul 23, 2024

View reviewed changes

tests/bootstrap.php Outdated Show resolved Hide resolved

westonruter added 2 commits July 22, 2024 21:36

Remove unnecessary casting

4f4f6bd

Fix logic inversion

eaf1f65

mukeshpanchal27 approved these changes Jul 23, 2024

View reviewed changes

westonruter merged commit de93b65 into trunk Jul 23, 2024
15 checks passed

westonruter deleted the add/phpstan-strict-rules branch July 23, 2024 05:10

This was referenced Jul 23, 2024

Eliminate use of empty() in Embed Optimizer and Optimization Detective #1391

Merged

Add strict PHPStan rules for Speculative Loading #1392

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHPStan strict rules (except for empty.notAllowed) #1241

Add PHPStan strict rules (except for empty.notAllowed) #1241

westonruter commented May 23, 2024 •

edited

Loading

westonruter commented Jun 13, 2024

westonruter Jun 13, 2024

github-actions bot commented Jun 13, 2024 •

edited

Loading

joemcgill left a comment

joemcgill Jul 11, 2024

westonruter Jul 11, 2024

joemcgill Jul 11, 2024

westonruter Jul 11, 2024

joemcgill Jul 11, 2024

joemcgill Jul 11, 2024

swissspidy left a comment

mukeshpanchal27 Jul 23, 2024

westonruter Jul 23, 2024

westonruter Jul 23, 2024

westonruter Jul 23, 2024

mukeshpanchal27 left a comment

mukeshpanchal27 commented Jul 23, 2024

	if ( is_string( $html_processor->get_attribute( 'type' ) ) ) {
	if ( null !== $html_processor->get_attribute( 'type' ) ) ) {

	if ( is_string( $href ) && '' !== $href ) {
	if ( null !== $href && '' !== $href ) {

	1 !== preg_match( '/(?:^\|\s)wp-image-([1-9]\d*)(?:\s\|$)/i', $class_name, $matches )
	1 !== (int) preg_match( '/(?:^\|\s)wp-image-([1-9]\d*)(?:\s\|$)/i', $class_name, $matches )

	if ( 0 === (int) preg_match_all( '/<(img)\s[^>]+>/', $content, $img_tags, PREG_SET_ORDER ) ) {
	if ( false === (bool) preg_match_all( '/<(img)\s[^>]+>/', $content, $img_tags, PREG_SET_ORDER ) ) {

Add PHPStan strict rules (except for empty.notAllowed) #1241

Add PHPStan strict rules (except for empty.notAllowed) #1241

Conversation

westonruter commented May 23, 2024 • edited Loading

westonruter commented Jun 13, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 13, 2024 • edited Loading

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

mukeshpanchal27 commented Jul 23, 2024

westonruter commented May 23, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading