#52217 - PHPStan config and fixes

[johnbillion]

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.
• ...

[hellofromtonya]

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]

@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]

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.

[johnbillion]

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

[johnbillion]

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

[samuelsolis]

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