Improve static analysis in Performance Lab plugin files #1188
Conversation
westonruter
added
[Type] Enhancement
| @@ -32,6 +34,8 @@ function perflab_add_features_page() { | |||
|
|
|||
| return $hook_suffix; | |||
| } | |||
|
|
|||
| // @phpstan-ignore-next-line | |||
There was a problem hiding this comment.
Because PHPStan identifies correctly that this function is used as an action, and not a filter. So it shouldn't return anything.
There was a problem hiding this comment.
The returned value is used in testing.
There was a problem hiding this comment.
Ideally the tests could be updated to not rely on this, but I think that is out of scope for this PR.
There was a problem hiding this comment.
| @@ -35,14 +37,16 @@ function perflab_add_server_timing_page() { | |||
|
|
|||
| return $hook_suffix; | |||
| } | |||
|
|
|||
| // @phpstan-ignore-next-line | |||
There was a problem hiding this comment.
Because PHPStan identifies correctly that this function is used as an action, and not a filter. So it shouldn't return anything.
| @@ -48,6 +50,8 @@ function perflab_server_timing() { | |||
|
|
|||
| return $server_timing; | |||
| } | |||
|
|
|||
| // @phpstan-ignore-next-line | |||
There was a problem hiding this comment.
Because PHPStan identifies correctly that this function is used as an action, and not a filter. So it shouldn't return anything.
westonruter
requested review from
manuelRod,
felixarntz,
mukeshpanchal27 and
adamsilverstein
as code owners
|
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. |
westonruter
force-pushed
the
improve/perflab-static-analysis
branch
from
39d14a9 to
468c49f
Compare
There was a problem hiding this comment.
Thanks @westonruter for the PR. overall look good to me.
I still get on one error.
Ignored error pattern #^Function str_starts_with not found\.$# in paths: /Users/mukesh/performance/plugins/optimization-detective/optimization.php, /Users/mukesh/performance/plugins/speculation-rules/class-plsr-url-pattern-prefixer.php was not matched in reported errors.
Ignored error pattern #^Function str_contains not found\.$# in paths: /Users/mukesh/performance/plugins/auto-sizes/hooks.php, /Users/mukesh/performance/plugins/dominant-color-images/hooks.php was not matched in reported errors.
|
It's because PHPStan targets the 7.2 and above functions are PHP 8.0+ compatible and WP adds polyfills for the above functions. Either we can ignore them in baseline or create stubs for them. We already have ignored them for a few files: Lines 18 to 30 in 9224f29 but I think it would be better if we can add stubs since more files can use it and PHPStan can read types from stubs. |
|
@thelovekesh so we add a new stubs file in |
|
That doesn't seem to work. Seems we have to just ignore the errors for now. |
|
@mukeshpanchal27 @thelovekesh I've updated the This is the stricter config I'm testing with: includes:
- phar://phpstan.phar/conf/bleedingEdge.neon
parameters:
level: 6
paths:
- load.php
- plugins/
- includes/
- tests/
treatPhpDocTypesAsCertain: false
checkGenericClassInNonGenericObjectType: false
reportUnmatchedIgnoredErrors: false
scanDirectories:
- vendor/wp-phpunit/wp-phpunit/
ignoreErrors:
# False positive as WordPress ships with a polyfill.
# Note that paths here don't work in PhpStorm due to <https://youtrack.jetbrains.com/issue/WI-63891>.
- "#^Function str_starts_with not found\\.$#"
- "#^Function str_contains not found\\.$#"
dynamicConstantNames:
- PERFLAB_OBJECT_CACHE_DROPIN_VERSION
bootstrapFiles:
- load.php
- bin/phpstan/constants.php
- plugins/webp-uploads/load.php |
|
I'd like to get this and #1195 merged sooner than later merge conflicts will just keep popping up, and they improve the quality of the codebase to build upon. |
Ideally, we should keep tools configs in a root level
@westonruter Are you scanning a stubs file for symbol declaration? I am doing it like this and it works for me. tools/phpstan/stubs.php<?php
if ( ! function_exists( 'str_starts_with' ) ) {
/**
* Returns true if the string starts with the needle, false otherwise.
*
* @param string $haystack The string to search in.
* @param string $needle The string to search for.
*
* @return bool
*/
function str_starts_with( $haystack, $needle ) { }
}
if ( ! function_exists( 'str_contains' ) ) {
/**
* Returns true if the string starts with the needle, false otherwise.
*
* @param string $haystack The string to search in.
* @param string $needle The string to search for.
*
* @return bool
*/
function str_contains( $haystack, $needle ) { }
}PHPStan configdiff --git a/phpstan.neon.dist b/phpstan.neon.dist
index 31ee0d46..2790ce58 100644
--- a/phpstan.neon.dist
+++ b/phpstan.neon.dist
@@ -11,12 +11,10 @@ parameters:
- load.php
- bin/phpstan/constants.php
- plugins/webp-uploads/load.php
+ scanFiles:
+ - tools/phpstan/stubs.php
scanDirectories:
- vendor/wp-phpunit/wp-phpunit/
dynamicConstantNames:
- PERFLAB_OBJECT_CACHE_DROPIN_VERSION
reportUnmatchedIgnoredErrors: false
- ignoreErrors:
- # False positive as WordPress ships with a polyfill.
- - "#^Function str_starts_with not found\\.$#"
- - "#^Function str_contains not found\\.$#" |
Oh, I was using |
I think that's for reading accurate PHPDoc for symbol declarations. Ref: https://phpstan.org/user-guide/stub-files |
|
Yeah, so then "stub" isn't technically the right term for this file, right? It's more of a compat file. |
Humm sounds right. |
|
OK, with 2cb9ab0 the |
| // The return value is validated in JavaScript at: | ||
| // <https://github.com/WordPress/wordpress-develop/blob/d1e0a6241dcc34f4a5ed464a741116461a88d43b/src/js/_enqueues/admin/site-health.js#L65-L114> | ||
| // If the value lacks the required keys of test, label, and description then it is omitted. | ||
| return array( 'omitted' => true ); |
There was a problem hiding this comment.
This is somewhat of a fix identified here through the use of stricter typing.
bin/phpstan/functions.stub
Outdated
There was a problem hiding this comment.
Should we keep the file extension as .php? So that PHPCS can lint it and have syntax highlighting in places like GitHub UI?
There was a problem hiding this comment.
I had it as a PHP file at first, but then static analysis complains. The use of .stub as the file extension is recommended in https://phpstan.org/user-guide/stub-files
There was a problem hiding this comment.
Valid point. In that case, can we include it in PHPCS config?
There was a problem hiding this comment.
I don't think it's necessary because then PHPCS will complain about this as well:
FILE: /home/westonruter/repos/performance/bin/phpstan/functions.stub
----------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------------------------
19 | ERROR | Function return type is not void, but function has no return statement (Squiz.Commenting.FunctionComment.InvalidNoReturn)
35 | ERROR | Function return type is not void, but function has no return statement (Squiz.Commenting.FunctionComment.InvalidNoReturn)
----------------------------------------------------------------------------------------------------------------------------------------
Time: 232ms; Memory: 14MB
| @@ -96,74 +108,104 @@ public function test_perflab_sanitize_server_timing_setting( $unsanitized, $expe | |||
| } | |||
|
|
|||
| public function data_perflab_sanitize_server_timing_setting() { | |||
There was a problem hiding this comment.
Changes in this data provider (and the test above) are due to https://github.com/WordPress/performance/pull/1188/files#r1589523090
There was a problem hiding this comment.
| if ( ! is_array( $value ) ) { | ||
| return array(); | ||
| $value = array(); | ||
| } | ||
|
|
||
| $value = array_intersect_key( $value, $allowed_keys ); | ||
| $value = wp_array_slice_assoc( | ||
| array_merge( perflab_get_server_timing_setting_default_value(), $value ), | ||
| array_keys( perflab_get_server_timing_setting_default_value() ) | ||
| ); |
There was a problem hiding this comment.
This ensures the default value is always stored, while also being the default option in case it is not stored.
| ?> | ||
| <style type="text/css"> |
There was a problem hiding this comment.
The type attribute is obsolete.
| check_admin_referer( 'perflab_aao_update_autoload' ); | ||
|
|
||
| if ( ! isset( $_GET['option_name'], $_GET['autoload'] ) ) { | ||
| wp_die( esc_html__( 'Missing required parameter.', 'performance-lab' ) ); | ||
| } | ||
|
|
||
| $option_name = sanitize_text_field( wp_unslash( $_GET['option_name'] ) ); | ||
| $autoload = isset( $_GET['autoload'] ) ? rest_sanitize_boolean( $_GET['autoload'] ) : false; | ||
| $autoload = rest_sanitize_boolean( $_GET['autoload'] ); |
There was a problem hiding this comment.
Note the removal of the isset( $_GET['autoload'] ) check since it is already checked above.
bin/phpstan/functions.stub
Outdated
| @@ -0,0 +1,38 @@ | |||
| <?php | |||
| /** | |||
| * Function stubs for PHPStan static analysis. | |||
There was a problem hiding this comment.
Does PHPStan require us to duplicate these, or can we configure them to be ignored since they are polyfilled in WP Core?
There was a problem hiding this comment.
I'm not entirely clear why these have to be duplicated. I think it's because wp-stubs doesn't include polyfilled functions? See php-stubs/wordpress-stubs#100
There was a problem hiding this comment.
I‘d just ignore the errors instead, personallly
There was a problem hiding this comment.
@swissspidy What about 828c87f?
The only issue I've seen with ignoring errors is that it causes problems when there are ignored errors which are not present in the subset of files being analyzed. Then you have to add reportUnmatchedIgnoredErrors: false. Clearly this doesn't matter if these two PHP8 functions are the only errors being ignored, but if you ignore other errors which no longer are present anymore then they could get ignored without being present.
There was a problem hiding this comment.
I don't fully understand the problem you are describing about reportUnmatchedIgnoredErrors, but 828c87f works for me.
| ignoreErrors: | ||
| # False positive as WordPress ships with a polyfill. | ||
| - | ||
| message: "#^Function str_starts_with not found\\.$#" | ||
| paths: | ||
| - load.php | ||
| - plugins/optimization-detective/optimization.php | ||
| - plugins/speculation-rules/class-plsr-url-pattern-prefixer.php | ||
| - | ||
| message: "#^Function str_contains not found\\.$#" | ||
| paths: | ||
| - plugins/auto-sizes/hooks.php | ||
| - plugins/dominant-color-images/hooks.php |
There was a problem hiding this comment.
I think we should keep this, but use /* to ignore these rules globally, rather than copying the shims to our repo.
There was a problem hiding this comment.
So like what was done in 51d1894?
There was a problem hiding this comment.
I think I have a better solution. See 828c87f
bin/phpstan/functions.stub
Outdated
| @@ -0,0 +1,38 @@ | |||
| <?php | |||
| /** | |||
| * Function stubs for PHPStan static analysis. | |||
There was a problem hiding this comment.
I don't fully understand the problem you are describing about reportUnmatchedIgnoredErrors, but 828c87f works for me.
| @@ -204,7 +219,7 @@ static function ( $hookname ) { | |||
| ); | |||
| } | |||
|
|
|||
| $value['output_buffering'] = ! empty( $value['output_buffering'] ); | |||
| $value['output_buffering'] = (bool) $value['output_buffering']; | |||
There was a problem hiding this comment.
interesting, I assume phpstan flagged this?
There was a problem hiding this comment.
Actually it's something I applied based on #1091 (comment)
| @@ -122,9 +122,9 @@ function perflab_aao_autoloaded_options_size() { | |||
| * | |||
| * @global wpdb $wpdb WordPress database abstraction object. | |||
| * | |||
| * @return array Autoloaded data as option names and their sizes. | |||
| * @return array<string, object{option_name: string, option_value_length: int}> Autoloaded data as option names and their sizes. | |||
There was a problem hiding this comment.
This is wrong. It is not an associative array. It should rather have been:
- * @return array<string, object{option_name: string, option_value_length: int}> Autoloaded data as option names and their sizes.
+ * @return array<object{option_name: string, option_value_length: int}> Autoloaded data as option names and their sizes.
westonruter
added
the
skip changelog
This fixes a bunch of static analysis issues identified by PHPStan, as well as a few from PhpStorm. This should go a long way to kickstart #775.
With PHPStan level 6, the following issues are resolved with this PR when running
composer phpstan load.php includes/*:PHPStan Output
The strict PHPStan config I'm using (in
phpstan.neonoverride):