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

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

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

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

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.

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,7 @@ on: @@
           - '.github/workflows/php-lint.yml'
           - '**.php'
           - 'phpcs.xml.dist'
+          - 'phpstan.neon.dist'
           - 'composer.json'
           - 'composer.lock'
       pull_request:
@@ Expand All / @@ -22,6 +23,7 @@ on: @@
           - '.github/workflows/php-lint.yml'
           - '**.php'
           - 'phpcs.xml.dist'
+          - 'phpstan.neon.dist'
           - 'composer.json'
           - 'composer.lock'
         types:
@@ Expand Down Expand Up / @@ -56,3 +58,5 @@ jobs: @@
             run: composer install --no-interaction --no-progress
           - name: PHP Lint
             run: composer lint
+          - name: PHPStan
+            run: composer phpstan

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

Diff view

Diff view

There are no files selected for viewing

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

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

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

Diff view

Diff view

There are no files selected for viewing

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

edited

Loading

swissspidy May 16, 2023 •

edited

Loading