#52217 - PHPStan config and fixes #853

johnbillion · 2021-01-04T22:25:25Z

Trac ticket: https://core.trac.wordpress.org/ticket/52217

This introduces config for PHPStan with plenty of exclusions so we can identify real issues that need to be fixed.

Changes:

Implement inline @var tags for variables assigned the return value of _get_list_table(). This is because the concrete return value of _get_list_table() cannot be known statically, and not all sub-classes of WP_List_Table use the same methods. This is essentially the same as directly instantiating the class passed to the $class parameter and means code editors as well as static analysis knows the correct class.
Correct the number of parameters passed to remove_filter()
Correct the default value for parameters of type callable. This makes the parameters nullable callables, instead of callable|string.
Correct the default value for parameters in the filesystem class constructors so they're always an array.
Remove the not-implemented touch() method from WP_Filesystem_SSH2. This method should return a boolean, so removing it means the touch() method from the parent class takes over and correctly returns boolean false.
...

phpstan.neon.dist

# Conflicts: # src/wp-admin/includes/class-wp-importer.php

…lysed.

jrfnl

While I'm not against the principle of adding more QA tools to our tooling chain, I'm not a fan of PHPStan for WordPress.

PHPStan - and Psalm as well for that matter - work best with well-typed code bases, the stricter the better.
WordPress is most definitely not such a code base, so the noise from false positives will be huge.

These are tools primarily aimed at modern PHP, not legacy codebases, and some of the results will include suggestions for modernizing code which cannot be applied to WP due to the low minimum PHP version we still have to deal with.
Also please realize that including the tooling could encourage people who are not as aware of the minimum PHP version for WP, to start sending in patches for things which cannot be applied in WP yet, causing wasted time both for the contributors who created the patches as well as for other maintainers having to explain - time and again - why the patch cannot be accepted.

I do think it's a good idea to do intermittent, ad-hoc runs with tools like PHPStan, Psalm, Exakat and other tools, but adding it as a dependency is a no-go, if for no other reason than the conflicting minimum PHP requirements.

I'm actually surprised the CI runs aren't breaking on this as it is, but haven't looked into that.

composer.json

jrfnl · 2021-07-15T18:34:30Z

src/wp-admin/edit-tags.php

 */
 global $post_type;

+/** @var WP_Terms_List_Table $wp_list_table */


What value do these type of comments add (other than to satisfy PHPstan) ?
(here and everywhere else in this patch)

Our list tables aren't LSP compatible, so any code editor or IDE that provides intellisense (in addition to PHPStan) raises errors further down when accessing methods or properties that only exist on certain WP_List_Table sub-classes. These inline type declarations avoid those errors.

Which, in my humble opinion, is doing things the wrong way around.

The LSP violations should be fixed and @hellofromtonya and me have been discussing the problem with those in our live stream sessions recently as well.

Adding comments to avoid addressing the real problem, especially comment which are only recognized by a select set of IDEs/tooling, just doesn't make any sense whatsoever to me.

That is indeed the preferred and correct way to address the problem, but realistically can that be fixed? And can it be enforced in future list tables? These classes are a free-for-all due to the dynamic method names used by columns and the arbitrary methods introduced in sub-classes.

There's nothing wrong with specifying the type of a variable in a PHPDoc where it cannot be determined statically. In this case it has the same effect as directly instantiating the concrete class instead of instantiating it via the _get_list_table() wrapper (which is little more than a non-standard implementation of an autoloader).

jrfnl · 2021-07-15T18:38:19Z

src/wp-admin/includes/class-custom-background.php

+	 * @param callable $admin_image_div_callback Optional. Custom image div output callback.
 	 */
-	public function __construct( $admin_header_callback = '', $admin_image_div_callback = '' ) {
+	public function __construct( $admin_header_callback = null, $admin_image_div_callback = null ) {


This could be considered a behavioral change, especially as the class is not final and may be extended in userland plugin/theme code.

To keep the existing behaviour, the property default values would need to be set to '' instead of changing the default values of the parameters in the constructor, but considering it is supposed to be a callable I agree that is counter-intuitive.

All the same, could you explain why the choice was made for doing it the other way around ?

Also: the places where these callables are used in the class do not use proper type checking. If anything, I'd advocate for the loose type if ( $this->admin_image_div_callback ) checks to be replaced with checks against is_callable().

Yes these classes can be extended, but the default value of an untyped parameter does not affect its signature, so it's safe to change this in the parent class and cause the sub-classes to diverge. Example: https://3v4l.org/8g5df

The choice to switch to a null default value is to make the parameter a nullable callable. A default value of '' means the parameter is actually callable|string and that doesn't help anybody. I agree that the loose checking could be enhanced with is_callable() checks, but that's outside the scope of the changes I want to make here.

I understand you want to limit the scope, but the is_callable() change is actually a useful change, while this isn't.

jrfnl · 2021-07-15T18:39:59Z

src/wp-admin/includes/class-custom-image-header.php

+	 * @param callable $admin_image_div_callback Optional. Custom image div output callback.
 	 */
-	public function __construct( $admin_header_callback, $admin_image_div_callback = '' ) {
+	public function __construct( $admin_header_callback, $admin_image_div_callback = null ) {


Same remark as made for src/wp-admin/includes/class-custom-background.php applies here.

src/wp-admin/includes/class-wp-filesystem-ftpext.php

jrfnl · 2021-07-15T22:47:29Z

src/wp-includes/wp-db.php

+	 * Undocumented variable
+	 *
+	 * @since ?.?.?
+	 * @var float


Blank line comment line needed between the above two tags (same applies to the $error property docblock).

Thanks! I need to finish these.

src/wp-mail.php

jrfnl · 2021-07-15T22:57:54Z

tests/phpstan/bootstrap.php

@@ -0,0 +1,100 @@
+<?php


Why is this placed in the tests directory ? This has nothing to do with testing WP.

Note: I realize @aaronjorbin and my comments about this are at odds with each other. Maybe a more general discussion should be had about what to do with tooling config files.

I'm not sure where best to put this, but static analysis is testing so I don't think it's an inappropriate location. Suggestions welcome though.

I think this belongs in the tests directory since I view static analysis as a form of testing (which seems to jive with @johnbillion since he said it just above me). I view automated testing as anything that automates a process of ensuring code quality.

jrfnl · 2021-07-15T23:01:50Z

tests/phpstan/bootstrap.php

@@ -0,0 +1,100 @@
+<?php
+// phpcs:ignoreFile
+


This list looks like it will quickly go out of date and add maintenance burden. Can a more dynamic solution be found for this ?

Good point but I think the maintenance burden is low. Constants are not introduced very often and this is not a complete list of all constants, just ones which are unconditionally accessed by the files that are scanned by PHPStan.

jrfnl · 2021-07-15T23:02:30Z

tests/phpstan/bootstrap.php

@@ -0,0 +1,100 @@
+<?php
+// phpcs:ignoreFile


This file should be added to the phpcs.xml.dist file for ignoring instead of having an inline comment.

hellofromtonya · 2021-07-16T16:59:34Z

Problems of using PHPStan as an inline inspection tool

PHPStan - and Psalm as well for that matter - work best with well-typed code bases, the stricter the better.
WordPress is most definitely not such a code base, so the noise from false positives will be huge.

I agree. I'd like to echo and add to @jrfnl's concerns.

PHPStan will generate a huge volume of false positives. Why is this a problem?

Significant effort will be needed to investigate each PHPStan incident to determine if it's a valid positive AND if it's something that could or should be changed for WordPress specifically without causing a backwards-compat issue.
I've witnessed this with the work being done in Trac ticket 51423. It's time-consuming work and the type of work that requires deep knowledge about not only PHP but also WordPress itself.
Risk of changing the codebase to make PHPStan happy without adding value to the codebase.
Bottlenecks in the workflow for contributors, maintainers, and committers.
Frustration pain point for contributors in figuring out how to make PHPStan happy.

Where does PHPStan fit into the project?

I do think it's a good idea to do intermittent, ad-hoc runs with tools like PHPStan, Psalm, Exakat and other tools

I agree. This tool and others can serve WordPress as a periodic auditing tool.

What about this PR?

There are valid fixes and improvements in this PR:

Some look like bug fixes
These could be broken out into separate tickets for tracking, history, and isolated discussion and testing.
Some are potential code improvements, though many need testing to ensure each is valid and does not cause a backwards compat issue.
These could be also be broken out from the PHPStan work as a separate ticket and PR.

By splitting out the work we can ensure these are not lost (along with the extensive code reviews) in the debate about whether to use PHPStan as an inline CI tool or not.

johnbillion · 2021-07-19T20:12:34Z

@hellofromtonya I've addressed your main concern about the impact of introducing static analysis in the comments on https://core.trac.wordpress.org/ticket/52217, but in short I don't believe that it introduces the sort of significant burden and large number of false positives that you've seen on https://core.trac.wordpress.org/ticket/51423 . This ticket aims to start at level 1, whereas 51423 aims to tackle type related problems that are several steps ahead of where we are now.

johnbillion · 2021-07-19T20:13:32Z

I'm going to update the description of this PR with notes about each change. I realise that it's not clear why quite a few of these changes have been made.

…kes care of returning boolean false in this case.

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>

…in [1543] and [6426].

# Conflicts: # composer.json # composer.lock

johnbillion · 2021-09-22T21:00:53Z

https://core.trac.wordpress.org/changeset/51850

# Conflicts: # src/wp-includes/class-wp-http-cookie.php # src/wp-includes/class.wp-dependencies.php # src/wp-includes/widgets.php # src/wp-includes/wp-db.php

johnbillion · 2021-09-22T21:25:26Z

https://core.trac.wordpress.org/changeset/51851

samuelsolis · 2023-10-11T13:07:00Z

This is an interesting point and seems easy to add to the project, what can we do to push it?

johnbillion added 11 commits January 4, 2021 00:16

Basic PHPStan configuration.

f117536

Start reducing the noise.

602cf9a

More noise reduction.

8754812

More enquietening.

9c6c482

Avoid some Multisite errors.

8dcb1aa

Formatting.

3b9cc08

Move onto level 1 analysis, ignore more files.

83db700

Docs.

6444bd7

Ignore WP_List_Table protected property access.

038a83c

More ignorance.

0f2d8d0

Formatting.

6b40bb1

aaronjorbin reviewed Jan 4, 2021

View reviewed changes

phpstan.neon.dist Outdated Show resolved Hide resolved

johnbillion added 18 commits January 5, 2021 17:53

Move the bootstrap file.

a9fec7d

Add a Composer script.

3f02f83

Correct the indentation rule for Neon files.

65416fc

Correct some calls to remove_filter().

f2efb03

Correct some class names.

75346da

These are too noisy at the moment.

dd878f9

Properly initialise some variables.

e0cb482

Unset the correct variable.

a763bb9

Merge branch 'master' into feature/phpstan

00c8bcc

# Conflicts: # src/wp-admin/includes/class-wp-importer.php

This is no longer needed.

9c63eb8

Merge branch 'master' into feature/phpstan

d6f1ba5

Update PHPStan.

8a4484c

This is too noisy at the moment.

34f2c7d

These aren't needed.

1d16f0f

Make this less specific so it catches uses in isset too.

1f089ab

Not worried about unused parameters for now.

226fffa

Reduce noise again.

c8f7eea

This $s is extracted in load_template().

0654bdd

johnbillion added 14 commits July 10, 2021 23:21

Don't need to sniff this.

786d69b

Cast some types that are used in calculations.

0a84206

Not much we can do about undocumented objects.

1f0a819

Fix the never returns.

608483d

Fix some default parameters.

7a4efb4

Fix some default parameter values.

b51bf00

Fix an inline type.

7cfde63

Document protected properties that are publicly exposed via __get().

7e15940

Fix some default parameter values.

f9b672d

Fix some return values.

e485b62

This should return, not echo.

ae4a869

Document protected properties that are publicly exposed via __get().

fc2d91d

Be more specific about which files are scanned and which are just ana…

e0985a9

…lysed.

Fix another default value.

0ce4e66

jrfnl requested changes Jul 15, 2021

View reviewed changes

johnbillion and others added 5 commits July 19, 2021 22:41

Replace this inline comment with proper PHPCS configuration.

27bbc1d

Remove this touch method as it's not implemented. The parent class ta…

f9a8d73

…kes care of returning boolean false in this case.

Apply suggestions from code review

1466583

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>

Correct the @since tags for these properties. They were introduced …

42a28b5

…in [1543] and [6426].

Merge branch 'master' into feature/phpstan

231430c

# Conflicts: # composer.json # composer.lock

Merge branch 'master' into feature/phpstan

1ba90e4

# Conflicts: # src/wp-includes/class-wp-http-cookie.php # src/wp-includes/class.wp-dependencies.php # src/wp-includes/widgets.php # src/wp-includes/wp-db.php

This was referenced Oct 21, 2024

#52217 - Refresh PR and update PHPStan johnbillion/wordpress-develop#4

Closed

Tests / Build Scripts: Configure PHPStan levels 1-6 [Exploratory] #7619

Open

johnbillion closed this May 15, 2025

#52217 - PHPStan config and fixes #853

#52217 - PHPStan config and fixes #853

Uh oh!

Conversation

johnbillion commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrfnl Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellofromtonya commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problems of using PHPStan as an inline inspection tool

Where does PHPStan fit into the project?

What about this PR?

Uh oh!

johnbillion commented Jul 19, 2021

Uh oh!

johnbillion commented Jul 19, 2021

Uh oh!

johnbillion commented Sep 22, 2021

Uh oh!

johnbillion commented Sep 22, 2021

Uh oh!

samuelsolis commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

johnbillion commented Jan 4, 2021 •

edited

Loading

jrfnl Jul 15, 2021 •

edited

Loading

hellofromtonya commented Jul 16, 2021 •

edited

Loading