Enhance code quality by adding PHPStan and fixing level 0 issues #730

westonruter · 2023-05-16T01:14:58Z

Summary

Fixes #727

Relevant technical choices

Add a phpstan composer script. This name is chosen instead of analyze since also spelled analyse.
Run PHPStan in the php-lint workflow after running PHPCS. This runs whenever a PHP-related file has changed.
Also run PHPStan in lint-staged whenever a PHP file is modified.
Add PHPStan config with szepeviktor/phpstan-wordpress and bleeding-edge, starting at level 0. Additional PRs will raise the level and fix more issues.
Fix issues uncovered by level 0.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…pin filter

…e phpdoc

tests/modules/images/webp-uploads/helper-tests.php

szepeviktor · 2023-05-16T10:02:42Z

Please consider reaching level 4.
Under level 4 there could be critical bugs and bad design decisions.

spacedmonkey · 2023-05-16T10:32:07Z

@westonruter Do this work in PHP 5.6? As I remember, phpstan requires PHP 7.2+

szepeviktor · 2023-05-16T10:35:28Z

PHPStan does static analysis, so it does not run the code.
Runtime PHP version can be set! https://phpstan.org/config-reference#phpversion

spacedmonkey · 2023-05-16T10:50:09Z

PHPStan does static analysis, so it does not run the code. Runtime PHP version can be set! https://phpstan.org/config-reference#phpversion

I am talking about composer installing and requires.

swissspidy · 2023-05-16T11:56:56Z

composer.json

@@ -14,8 +14,11 @@
  "require-dev": {
    "dealerdirect/phpcodesniffer-composer-installer": "^0.7",
    "phpcompatibility/php-compatibility": "^9.3",
+    "phpstan/extension-installer": "^1.3.0",
+    "phpstan/phpstan": "^1.10",


Technically not needed because it's a dependency of szepeviktor/phpstan-wordpress already

Yes, I thought about that. But I thought this might get us PHPStan updates more quickly by being an explicit dependency. Otherwise we'd need to wait for the package to update, AFAIK.

swissspidy · 2023-05-16T12:02:19Z

composer.json

    "phpunit/phpunit": "^4|^5|^6|^7|^8|^9",
    "squizlabs/php_codesniffer": "^3.5",
+    "szepeviktor/phpstan-wordpress": "^1.3.0",


Not that this package and PHPStan itself require PHP 7.2.

So when we start running tests against PHP 5.6 (see #399), composer install would break.

The plugin itself wouldn't break, as it doesn't use the autoloader.

Same discussion as in #729 really.

Check out WordPress/plugin-check#166 for a workaround by moving these deps to a separate composer.json file.

Great point @swissspidy, let's apply the same approach here as we want to support PHP 5.6 too for our test suite as we should preferably run the tests on that version too.

As I understand, there are three possible ways to about this:

Have a separate composer file for PHP 7.2+. (Just seeing Improve PHP cross-version support and testing plugin-check#166 now.)

In GHA workflow, just-in-time composer require this package before doing composer install before running PHPCS

In GHA workflow, just-in-time composer remove this package before doing composer install prior to running PHPUnit in PHP<7.2.

I think the third option is ideal, since all normal development environments would be running PHP 7.2+. It's only the exception case, specifically on GHA, where PHP 5.6 is needed. This would allow a developer to have all packages installed normally.

The first option doesn't seem ideal since there would be a separate composer file to maintain, and Dependabot wouldn't automatically be aware of it.

How about we address back-compat with PHP 5.6 as part of #399?

Downsides of (2) and (3) are that it's a less reproducible setup for developers, and that it's cumbersome to do for other packages as well (think all the PHPStan extensions or the Slevomat CS package). I did something like that in the past for downgrading/upgrading PHPUnit in CI, but it's not more maintainable than (1).

I wish (1) wasn't needed, but when dealing with such version requirements it's a necessary evil.

Dependabot wouldn't automatically be aware of it.

FWIW this is easily solved with another entry in the Dependabot config. Example:

- package-ecosystem: "composer" directory: "/" schedule: interval: "daily" - package-ecosystem: "composer" directory: "/build-cs" schedule: interval: "daily"

3. In GHA workflow, just-in-time composer remove this package before doing composer install prior to running PHPUnit in PHP<7.2.

I just noticed that Gutenberg actually takes this approach instead of maintaining a separate composer.json.

How is this less maintainable than maintaining a separate composer.json?

swissspidy · 2023-05-16T12:04:15Z

composer.json

    "phpunit/phpunit": "^4|^5|^6|^7|^8|^9",
    "squizlabs/php_codesniffer": "^3.5",
+    "szepeviktor/phpstan-wordpress": "^1.3.0",


I can also highly recommend https://packagist.org/packages/swissspidy/phpstan-no-private to catch calls of seemingly private functions.

Plus https://packagist.org/packages/phpstan/phpstan-phpunit and https://packagist.org/packages/php-stubs/wordpress-tests-stubs to run PHPStan against tests as well. Believe me, it really improves the quality of the tests!

https://packagist.org/packages/phpstan/phpstan-deprecation-rules is also nice to prevent usage of deprecated functions.

I can also highly recommend https://packagist.org/packages/swissspidy/phpstan-no-private to catch calls of seemingly private functions.

I love this package, but I wonder if this is the best for Performance Lab considering that the code in question will end up in core and thus it is expected that many calls to internal/private functions will be made? As it stands right now...

There are 73 instances of this

------ ------------------------------------------------------- Line modules/database/sqlite/activate.php ------ ------------------------------------------------------- 71 Call to private/internal function wp_set_wpdb_vars(). 111 Call to private/internal function wp_set_wpdb_vars(). ------ ------------------------------------------------------- ------ ------------------------------------------------------------------------------------------------ Line modules/database/sqlite/wp-includes/sqlite/class-perflab-sqlite-alter-query.php ------ ------------------------------------------------------------------------------------------------ 53 Call to private/internal method command_tokenizer() of class Perflab_SQLite_Alter_Query. 65 Call to private/internal method handle_single_command() of class Perflab_SQLite_Alter_Query. 69 Call to private/internal method handle_add_primary_key() of class Perflab_SQLite_Alter_Query. 73 Call to private/internal method handle_drop_primary_key() of class Perflab_SQLite_Alter_Query. 77 Call to private/internal method handle_modify_command() of class Perflab_SQLite_Alter_Query. 81 Call to private/internal method handle_change_command() of class Perflab_SQLite_Alter_Query. 85 Call to private/internal method handle_alter_command() of class Perflab_SQLite_Alter_Query. 261 Call to private/internal method convert_field_types() of class Perflab_SQLite_Alter_Query. 367 Call to private/internal method convert_field_types() of class Perflab_SQLite_Alter_Query. 421 Call to private/internal method convert_field_types() of class Perflab_SQLite_Alter_Query. 483 Call to private/internal method convert_field_types() of class Perflab_SQLite_Alter_Query. 500 Call to private/internal method convert_field_types() of class Perflab_SQLite_Alter_Query. 513 Call to private/internal method convert_field_types() of class Perflab_SQLite_Alter_Query. ------ ------------------------------------------------------------------------------------------------ ------ ----------------------------------------------------------------------------------------------- Line modules/database/sqlite/wp-includes/sqlite/class-perflab-sqlite-create-query.php ------ ----------------------------------------------------------------------------------------------- 76 Call to private/internal method strip_backticks() of class Perflab_SQLite_Create_Query. 77 Call to private/internal method quote_illegal_field() of class Perflab_SQLite_Create_Query. 78 Call to private/internal method get_table_name() of class Perflab_SQLite_Create_Query. 79 Call to private/internal method rewrite_comments() of class Perflab_SQLite_Create_Query. 80 Call to private/internal method rewrite_field_types() of class Perflab_SQLite_Create_Query. 81 Call to private/internal method rewrite_character_set() of class Perflab_SQLite_Create_Query. 82 Call to private/internal method rewrite_engine_info() of class Perflab_SQLite_Create_Query. 83 Call to private/internal method rewrite_unsigned() of class Perflab_SQLite_Create_Query. 84 Call to private/internal method rewrite_autoincrement() of class Perflab_SQLite_Create_Query. 85 Call to private/internal method rewrite_primary_key() of class Perflab_SQLite_Create_Query. 86 Call to private/internal method rewrite_foreign_key() of class Perflab_SQLite_Create_Query. 87 Call to private/internal method rewrite_unique_key() of class Perflab_SQLite_Create_Query. 88 Call to private/internal method rewrite_enum() of class Perflab_SQLite_Create_Query. 89 Call to private/internal method rewrite_set() of class Perflab_SQLite_Create_Query. 90 Call to private/internal method rewrite_key() of class Perflab_SQLite_Create_Query. 91 Call to private/internal method add_if_not_exists() of class Perflab_SQLite_Create_Query. 93 Call to private/internal method post_process() of class Perflab_SQLite_Create_Query. ------ ----------------------------------------------------------------------------------------------- ------ ---------------------------------------------------------------------------------------------------- Line modules/database/sqlite/wp-includes/sqlite/class-perflab-sqlite-pdo-driver.php ------ ---------------------------------------------------------------------------------------------------- 81 Call to private/internal method parse_query() of class Perflab_SQLite_PDO_Driver. 84 Call to private/internal method handle_truncate_query() of class Perflab_SQLite_PDO_Driver. 88 Call to private/internal method handle_alter_query() of class Perflab_SQLite_PDO_Driver. 92 Call to private/internal method handle_create_query() of class Perflab_SQLite_PDO_Driver. 97 Call to private/internal method handle_describe_query() of class Perflab_SQLite_PDO_Driver. 101 Call to private/internal method handle_show_query() of class Perflab_SQLite_PDO_Driver. 105 Call to private/internal method handle_show_columns_query() of class Perflab_SQLite_PDO_Driver. 109 Call to private/internal method handle_show_index() of class Perflab_SQLite_PDO_Driver. 115 Call to private/internal method rewrite_date_sub() of class Perflab_SQLite_PDO_Driver. 116 Call to private/internal method delete_index_hints() of class Perflab_SQLite_PDO_Driver. 117 Call to private/internal method rewrite_regexp() of class Perflab_SQLite_PDO_Driver. 119 Call to private/internal method fix_date_quoting() of class Perflab_SQLite_PDO_Driver. 120 Call to private/internal method rewrite_between() of class Perflab_SQLite_PDO_Driver. 121 Call to private/internal method handle_orderby_field() of class Perflab_SQLite_PDO_Driver. 126 Call to private/internal method execute_duplicate_key_update() of class Perflab_SQLite_PDO_Driver. 127 Call to private/internal method rewrite_insert_ignore() of class Perflab_SQLite_PDO_Driver. 128 Call to private/internal method rewrite_regexp() of class Perflab_SQLite_PDO_Driver. 129 Call to private/internal method fix_date_quoting() of class Perflab_SQLite_PDO_Driver. 134 Call to private/internal method rewrite_update_ignore() of class Perflab_SQLite_PDO_Driver. 136 Call to private/internal method rewrite_limit_usage() of class Perflab_SQLite_PDO_Driver. 137 Call to private/internal method rewrite_order_by_usage() of class Perflab_SQLite_PDO_Driver. 138 Call to private/internal method rewrite_regexp() of class Perflab_SQLite_PDO_Driver. 139 Call to private/internal method rewrite_between() of class Perflab_SQLite_PDO_Driver. 144 Call to private/internal method rewrite_limit_usage() of class Perflab_SQLite_PDO_Driver. 145 Call to private/internal method rewrite_order_by_usage() of class Perflab_SQLite_PDO_Driver. 146 Call to private/internal method rewrite_date_sub() of class Perflab_SQLite_PDO_Driver. 147 Call to private/internal method rewrite_regexp() of class Perflab_SQLite_PDO_Driver. 148 Call to private/internal method delete_workaround() of class Perflab_SQLite_PDO_Driver. 153 Call to private/internal method rewrite_date_sub() of class Perflab_SQLite_PDO_Driver. 154 Call to private/internal method rewrite_regexp() of class Perflab_SQLite_PDO_Driver. 158 Call to private/internal method rewrite_optimize() of class Perflab_SQLite_PDO_Driver. 168 Call to private/internal method return_true() of class Perflab_SQLite_PDO_Driver. ------ ---------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------------------------------------------------- Line modules/database/sqlite/wp-includes/sqlite/class-perflab-sqlite-pdo-engine.php ------ ------------------------------------------------------------------------------------------------------- 1260 Call to private/internal method convert_to_columns_object() of class Perflab_SQLite_PDO_Engine. 1262 Call to private/internal method convert_to_index_object() of class Perflab_SQLite_PDO_Engine. 1264 Call to private/internal method convert_result_check_or_analyze() of class Perflab_SQLite_PDO_Engine. ------ ------------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------------------------------------------------- Line modules/database/sqlite/wp-includes/sqlite/class-perflab-sqlite-pdo-user-defined-functions.php ------ ------------------------------------------------------------------------------------------------------- 324 Call to private/internal method derive_interval() of class Perflab_SQLite_PDO_User_Defined_Functions. 356 Call to private/internal method derive_interval() of class Perflab_SQLite_PDO_User_Defined_Functions. ------ ------------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------------------------ Line modules/images/dominant-color-images/hooks.php ------ ------------------------------------------------------------------------------ 19 Call to private/internal function _dominant_color_get_dominant_color_data(). ------ ------------------------------------------------------------------------------ ------ ------------------------------------------------------------------------------------ Line modules/images/webp-uploads/helper.php ------ ------------------------------------------------------------------------------------ 222 Call to private/internal function webp_uploads_generate_additional_image_source(). ------ ------------------------------------------------------------------------------------ ------ ------------------------------------------------------------------------------------ Line modules/images/webp-uploads/hooks.php ------ ------------------------------------------------------------------------------------ 82 Call to private/internal function webp_uploads_generate_additional_image_source(). 126 Call to private/internal function _wp_image_meta_replace_original(). ------ ------------------------------------------------------------------------------------

Plus https://packagist.org/packages/phpstan/phpstan-phpunit

Thanks for that. Added in 916dd25, fixing 2 found issues in 5e498d1.

and https://packagist.org/packages/php-stubs/wordpress-tests-stubs

Do we need the test subs since we're already installing wp-phpunit/wp-phpunit? Would this essentially be an alternative to:

performance/phpstan.neon.dist

Lines 14 to 15 in 5e498d1

scanDirectories:

- vendor/wp-phpunit/wp-phpunit/

https://packagist.org/packages/phpstan/phpstan-deprecation-rules is also nice to prevent usage of deprecated functions.

Added in 4554f6a, issues fixed in bd1c378.

I love this package, but I wonder if this is the best for for Performance Lab considering that the code in question will end up in core and thus it is expected that many calls to internal/private functions will be made? As it stands right now...

Good point! 😃

I think it can't hurt to use it anyway, since not everything will land in core. Places where usage is OK can be ignored in the PHPStan config accordingly. But maybe in another PR :-)

All the Perflab_SQLite_ reports are actually false positives! I'll get this fixed in a new release. (Edit: see swissspidy/phpstan-no-private#2)
They are not properly documented though. A private function ...() method does not need @access private annotations. That's against the coding standards.

Do we need the test subs since we're already installing wp-phpunit/wp-phpunit? Would this essentially be an alternative to:

Probably not in that case. Counter question: why is wp-phpunit even used here when all tests are running in wp-env anyway?

Agreed, also if our code here follows PHPStan, I don't think it would violate anything that core requires - we'd effectively have stricter requirements than core, which I think should work fine as long as it's in that direction and not the opposite :)

why is wp-phpunit even used here when all tests are running in wp-env anyway?

I believe that is for additional flexibility. We don't want to dictate anyone to use wp-env, it's just the easy way to go. If folks want to e.g. run a DB on their own machine, the use of wp-phpunit makes that cleaner and decouples from having to have the entire WP core dev environment.

westonruter · 2023-05-16T15:11:12Z

Please consider reaching level 4. Under level 4 there could be critical bugs and bad design decisions.

@szepeviktor yes, that's the plan. Just for the initial PR level 0 is the first target.

felixarntz · 2023-05-30T18:41:02Z

@westonruter @swissspidy Could you please follow up on the discussions in this PR to figure out how to move it forward? Not critical, but since it's milestoned for 2.4.0 and stale for 2 weeks now, it would be great to keep this moving.

westonruter · 2023-05-30T20:02:43Z

@felixarntz I think the only current holdup is #730 (comment) and how we deal with Composer dependencies that aren't compatible with 5.6. However, since tests don't currently run in 5.6, I think that issue should be punted for when #399 is addressed.

felixarntz · 2023-05-30T20:06:10Z

@westonruter That works for me, given #399 is not currently milestoned and it seems to me this PR here is closer to completion than #544 (which is from September 2022 so likely stale at least to some degree).

westonruter · 2023-05-31T17:20:05Z

I think this just needs an approval than so it can be merged.

felixarntz

Thanks @westonruter, LGTM!

felixarntz · 2023-05-31T20:49:35Z

@westonruter

I think this just needs an approval than so it can be merged.

I've approved it, but every PR needs 2+ approvals, so let's wait for @swissspidy to give it another look next week.

westonruter · 2023-05-31T22:11:42Z

Once this is merged we should open a PR that bumps PHPStan to level 1 and higher.

swissspidy · 2023-06-13T11:48:50Z

@westonruter Mind creating the follow-up tickets for this?

westonruter · 2023-07-10T22:39:52Z

Once this is merged we should open a PR that bumps PHPStan to level 1 and higher.

See #775

I can also highly recommend https://packagist.org/packages/swissspidy/phpstan-no-private to catch calls of seemingly private functions.

See #776

westonruter added 2 commits May 15, 2023 18:12

Require phpstan packages

7456d9d

Add package scripts and lint-staged for phpstan

1e298c3

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure labels May 16, 2023

westonruter added this to the 2.4.0 milestone May 16, 2023

westonruter added 9 commits May 15, 2023 18:16

Add PHPStan to php-lint workflow

0e5afc1

Add phpstan config

4be88c6

Run composer update

45b4573

Add phpstan config to branches.paths in php-lint workflow

216985b

Add phpstan/extension-installer to allow-plugins

bcb4963

Add missing parameter in phpdocc for perflab_disable_object_cache_dro…

69630a0

…pin filter

Fix composer script name in lint-staged

dd72fff

Add missing return value null for void

07c43e7

Add missing default value for message param in assertImageNotHasSourc…

c0db055

…e phpdoc

westonruter changed the title ~~Add PHPStan~~ Add PHPStan and fix level 0 issues May 16, 2023

westonruter marked this pull request as ready for review May 16, 2023 02:49

westonruter requested review from adamsilverstein, felixarntz, mitogh and JustinyAhin as code owners May 16, 2023 02:49

westonruter requested a review from spacedmonkey May 16, 2023 02:56

aristath reviewed May 16, 2023

View reviewed changes

tests/modules/images/webp-uploads/helper-tests.php Outdated Show resolved Hide resolved

swissspidy reviewed May 16, 2023

View reviewed changes

felixarntz mentioned this pull request May 16, 2023

Use static closures for minor performance improvement whenever instance access is not needed #729

Merged

3 tasks

Require phpstan/phpstan-phpunit

916dd25

Fix covers tags and a couple typos

5e498d1

westonruter requested a review from pbearne as a code owner May 16, 2023 16:43

westonruter and others added 3 commits May 16, 2023 09:48

Require phpstan/phpstan-deprecation-rules

4554f6a

Replace deprecated RegExp assertions

bd1c378

Reuse __return_null

93ec3dc

Merge branch 'trunk' into add/phpstan

789f317

felixarntz approved these changes May 31, 2023

View reviewed changes

felixarntz requested a review from swissspidy May 31, 2023 20:49

swissspidy approved these changes Jun 13, 2023

View reviewed changes

swissspidy merged commit 92338e3 into trunk Jun 13, 2023

swissspidy deleted the add/phpstan branch June 13, 2023 11:48

felixarntz changed the title ~~Add PHPStan and fix level 0 issues~~ Enhance code quality by adding PHPStan and fixing level 0 issues Jun 13, 2023

This was referenced Jul 10, 2023

Increase PHPStan to rule level to 1 (and beyond) #775

Closed

Add phpstan-no-private to catch calls of seemingly private functions #776

Open

felixarntz mentioned this pull request Sep 7, 2023

Remove PHPStan config file from plugin directory #816

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance code quality by adding PHPStan and fixing level 0 issues #730

Enhance code quality by adding PHPStan and fixing level 0 issues #730

westonruter commented May 16, 2023 •

edited

Loading

szepeviktor commented May 16, 2023

spacedmonkey commented May 16, 2023

szepeviktor commented May 16, 2023

spacedmonkey commented May 16, 2023

swissspidy May 16, 2023

westonruter May 16, 2023

swissspidy May 16, 2023

felixarntz May 16, 2023

westonruter May 16, 2023

westonruter May 16, 2023

swissspidy May 16, 2023

westonruter May 19, 2023

westonruter May 19, 2023

swissspidy May 16, 2023

westonruter May 16, 2023 •

edited

Loading

westonruter May 16, 2023

westonruter May 16, 2023

westonruter May 16, 2023

swissspidy May 16, 2023 •

edited

Loading

felixarntz May 16, 2023

westonruter commented May 16, 2023

felixarntz commented May 30, 2023

westonruter commented May 30, 2023

felixarntz commented May 30, 2023

westonruter commented May 31, 2023

felixarntz left a comment

felixarntz commented May 31, 2023

westonruter commented May 31, 2023 •

edited

Loading

swissspidy commented Jun 13, 2023

westonruter commented Jul 10, 2023

Enhance code quality by adding PHPStan and fixing level 0 issues #730

Enhance code quality by adding PHPStan and fixing level 0 issues #730

Conversation

westonruter commented May 16, 2023 • edited Loading

Summary

Relevant technical choices

Checklist

szepeviktor commented May 16, 2023

spacedmonkey commented May 16, 2023

szepeviktor commented May 16, 2023

spacedmonkey commented May 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter May 16, 2023 • edited Loading

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 May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented May 16, 2023

felixarntz commented May 30, 2023

westonruter commented May 30, 2023

felixarntz commented May 30, 2023

westonruter commented May 31, 2023

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented May 31, 2023

westonruter commented May 31, 2023 • edited Loading

swissspidy commented Jun 13, 2023

westonruter commented Jul 10, 2023

westonruter commented May 16, 2023 •

edited

Loading

westonruter May 16, 2023 •

edited

Loading

swissspidy May 16, 2023 •

edited

Loading

westonruter commented May 31, 2023 •

edited

Loading