Fix code issues identified by PHPStan

[johnbillion]

In #51423 several issues were identified with function argument types that were detected by PHPStan.

There are other issues that PHPStan detects too such as potentially undefined variables, functions called with incorrect number of parameters, access to undefined properties, and more as you increase the level of checking performed.

We can scan core using the level 1 checks and catch quite a few low hanging fruit, and then go from there.

Follow-up task: Consider whether it's beneficial to add PHPStan (or Psalm or Phan) analysis to the build tooling and CI.

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.

[jorbin]

Left one comment about where we put the bootstrap file, otherwise, love this idea.

Consider whether it's beneficial to add PHPStan (or Psalm or Phan) analysis to the build tooling and CI.

I lean towards yes

[szepe.viktor]

to add PHPStan (or Psalm or Phan) analysis to the build tooling and CI.

Let's lean towards PHPStan.

written by a PHPStan Advocate

[johnbillion]

In 49936:

Docs: Corrections and improvements to types used in various docblocks.

See #51800, #52217

[johnbillion]

In 49946:

Plugins: Replace usage of $this in action and filter parameter docblocks with more appropriate variable names.

See #51800, #52217

Fixes #52243

[jrf]

FYI: I've done a detailed review of the patch on GH and left extensive comments.

I don't think this can be introduced as a dependency, if for no other reason than a mismatch in minimum PHP requirements.

Valid fixes found through scans with PHPStan, however, should, of course, be committed.

[prbot]

​hellofromtonya commented on ​PR #853:

### Problems of using PHPStan as an inline continuous 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.

[hellofromTonya]

Consider whether it's beneficial to add PHPStan (or Psalm or Phan) analysis to the build tooling and CI.

Using it as a periodic auditing tool is valuable for the project.

Caution: Using it will be expensive in terms of effort and time. Why? PHPStan will generate significant false positives. Many of these will not be fixable in WordPress due to backwards-compat. Many may not be valid for WordPress. As a result, each incident reported by PHPStan requires a deep review and knowledge to do the review to determine if it's a valid report that needs fixing.

I agree with @jrf that it's not well suited as part of an inline CI workflow that runs on every PR. ​Left a comment in the PR to share reasoning.

[hellofromTonya]

Related #51423

[szepe.viktor]

Replying to hellofromTonya:

Caution: Using it will be expensive in terms of effort and time.

PHPStan has a feature called "baseline" all false positives, todos, anything can go into the baseline file.
It makes it possible to keep further development at high quality.

[johnbillion]

Thanks for the comments! I'll go through the comments on the PR shortly and respond there, but I just wanted to say that any additional tooling or testing we introduce into WordPress will require significant configuration and, as evidenced by the phpstan.neon.dist file in my PR, a significant number of exceptions. This should not be a reason to discount introducing tooling that can identify bugs.

As Viktor mentioned, PHPStan has a specific solution for introducing its analysis into a legacy codebase. You can generate a baseline report which represents the current state of the project, and then you can configure the level of checking for any newly introduced code. Docs: ​https://phpstan.org/user-guide/baseline.

PHPStan works great on strictly typed codebases but it does much more than type checking. In fact it doesn't do any type checking until you configure it to use level 3 or higher. At level 1, which I've used in my PR, it checks for unknown symbols, wrong numbers of arguments, and undefined variables. Docs: ​https://phpstan.org/user-guide/rule-levels.

All of the errors that are flagged by PHPStan at level 0 and 1 are legitimate code errors that need to be fixed. Only once you move into level 2 and above does it tackle errors that are based on the validity of type hints, return types, and PHPDoc tag types.

Ticket #51423 is a rabbit hole. This ticket is much more narrowly focused.

[prbot]

​johnbillion commented on ​PR #853:

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

[prbot]

​johnbillion commented on ​PR #853:

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.

[hellofromTonya]

Replying to johnbillion:

This should not be a reason to discount introducing tooling that can identify bugs.

I agree that it can help identify bugs, which is why we saw it as a good ad-hoc and auditing tool. I wonder if I might have misunderstood how this ticket is proposing it be used. So let me ask:

@johnbillion Do you envision wiring PHPStan as another inline CI dependency like we do for running phpcs and phpunit?

If yes:

• How do we help contributors analyze incidents reported by it to determine if it's a real and fixable issue and then resolve each?
• Do you foresee it becoming a pain point for contributors?

[johnbillion]

My original proposal was going to be to configure and run PHPStan on core (at level 1 or 2) and fix the issues it raises, then circle back at a later point to consider whether implementing it as part of our tooling and CI would be a good idea. Jorbin and Viktor made positive sounding noises in the comments above so I just went with it.

From my experience with PHPStan at levels 0 and 1, once you get the exclusions and configuration in place that you need then it only warns about real code problems, and as I mentioned it won't consider anything relating to types.

How do we help contributors analyze incidents reported by it to determine if it's a real and fixable issue and then resolve each?

I would say the PHPStan error messages are similar in terms of technical complexity and language to PHPCS. The errors you'll see from levels 0 and 1 relate to undefined variables, unknown functions and methods, and incorrect parameter counts. I would say these are on par with PHPCS in terms of how experienced you need to be to understand and fix the problem.

PHPStan has a documentation section specifically for writing PHP code correctly, for example ​solving undefined variables.

Do you foresee it becoming a pain point for contributors?

Potentially yes, but I'm of the opinion that errors from ​levels 0 and 1 are manageable now the initial configuration is in place. It's not going to report anything opinionated, nor is it going to report problems with types.

From reading through the conversation and PRs on #51423 I think it's trying to tackle too much at once and is probably the source of the pain points you mention. In that situation I don't think implementing PHPStan at the level that reports those problems is realistic, certainly not at the moment anyway.

I'm very happy to fix the actual errors that I've seen at level 1 and then open a separate ticket for implementing PHPStan into the tooling, it was just easier and more accessible to implement it all together in my PR.

[johnbillion]

In 51850:

General: Fix code quality issues which were identified by static analysis.

This fixes minor issues that could cause PHP notices under the right conditions, and fixes some general incorrectness.

Props jrf, hellofromTonya for review

See #52217

[prbot]

​johnbillion commented on ​PR #853:

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

[johnbillion]

In 51851:

Docs: Miscellaneous docblock corrections and improvements.

See #52217, #53399

[prbot]

​johnbillion commented on ​PR #853:

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

[hellofromTonya]

With 5.9 feature freeze tomorrow and Beta next week, moving this ticket to Future Release (as the next milestone is not yet opened). @johnbillion please feel free to move it into a future milestone.