Make WordPress Core

Opened 19 months ago

Last modified 6 weeks ago

#61175 assigned enhancement

Integrate PHPStan into the core development workflow

Reported by: westonruter's profile westonruter Owned by: justlevine's profile justlevine
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests changes-requested
Focuses: Cc:

Description

PHPStan is a vital static analysis tool for identifying bugs in code. Previously it has been used to identify problems in core by manually running PHPStan on the core codebase:

As @johnbillion suggests in #52217:

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

I suggest we incorporate PHPStan now into the workflow with a populated baseline that captures all of the existing issues and ignores them so that everything doesn't have to be fixed up front. This baseline will then allow new issues to be reported as they are introduced in the codebase, again without having to fix everything up-front.

We can start with either rule level 0 or 1 and then go from there as we fix issues in the codebase. It may not make sense to implement the highest levels.

For reference, the Performance team has implemented PHPStan as part of the Performance Lab codebase and there have been separate PRs fixing issues for each level (1, 2, 3, 4, 5, 6, 7). It is remarkable how effective it is at identifying problems.

PHPStan should be run alongside PHPCS in GHA and locally as part of the pre-commit checks.

Change History (53)

#1 follow-up: @jorbin
19 months ago

There is a PR in https://github.com/WordPress/wordpress-develop/pull/853/files that is stalled that I think could serve as the starting point once the discussions there can come to a satisfactory conclusion.

#2 @oglekler
19 months ago

We have 1 week before Beta 1, and it looks like patch is not exactly ready. If adding PHPStan itself is not affecting code directly, then changing some default values can. I propose to split PHPStan addition and suggested code fixes.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


18 months ago

#4 @oglekler
18 months ago

  • Milestone changed from 6.6 to 6.7

We have just a few days before Beta 1, so, I am moving this to the next milestone for further investigation.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


15 months ago

#6 @chaion07
15 months ago

Thanks @westonruter for reporting this. We reviewed this ticket during a recent bug-scrub session.

We received the following feedback:

  1. To work closely with the Performance Team for more solutions
  2. As some discussions are from 2021 and back, we feel the need for some confirmation

Props to @khoipro for the suggestion

Cheers

#7 @desrosj
14 months ago

  • Milestone changed from 6.7 to Future Release

With no meaningful progress during the 6.7 release cycle, I'm goign to punt this one.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


14 months ago

#9 in reply to: ↑ 1 @justlevine
14 months ago

Replying to jorbin:

There is a PR in https://github.com/WordPress/wordpress-develop/pull/853/files that is stalled that I think could serve as the starting point once the discussions there can come to a satisfactory conclusion.

I opened up https://github.com/johnbillion/wordpress-develop/pull/4 to refresh the above PR. It merges in the latest trunk and updates PHPStan to v1.12.7

I plan to go through the remaining list of exposed issues, and see what can be addressed in isolation towards #52217 and #59653, but am also happy to help advance this ticket directly.

(I'm wondering if this should perhaps be distilled from PR-853, so it only contains config + annotations instead of code changes. Hopefully I'll understand more once I finish reviewing all the PR comments).

This ticket was mentioned in PR #7619 on WordPress/wordpress-develop by @justlevine.


14 months ago
#10

  • Keywords has-patch has-unit-tests added; needs-patch removed

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

This PR adds a PHPStan configuration along with error baselines through Level 6.

The hope is the limited scope will make this easier to review/merge, with actual code remediations occuring in future (or even parallel) PRs.

## Usage

### Run PHPStan

composer run analyse

# Generate a report
# https://phpstan.org/user-guide/output-format
composer run analyse -- --error-format=checkstyle

### Create a local phpstan.neon for remediating errors

  1. Copy phpstan.neon.dist to phpstan.neon.
  2. Comment out the unnecessary baselines, change the parameters.level to the desired level, and filter out the errors by adding them to the parameters.ignoreErrors list.
  3. Run PHPStan as usual: composer run analyse.

### Remediating errors using the error baselines

While parameters.ignoreErrors is used to filter out "unsupported" errors, error baselines are used to suppress _preexisting_ tech debt.

This allows for the PR to be merged as is to enforce the rules on new code, while allowing contributors to remediate the existing errors at their own pace. This is in the spirit of 'Avoid unnecessary refactoring'.

To remediate a baselined error, remove the error from the tests/phpstan/baseline/level-{%N} file and run PHPStan again.

#### Triaging errors and regenerating baselines

If an error is found to be a false positive (or otherwise not worth fixing), it should be added to the parameters.ignoreErrors list in the phpstan.neon.dist file. When this happens, the baseline file suppressing the error will cause PHPStan to fail.

To avoid manual remediation, the baseline files can be regenerated by following the following steps:

  1. Identify the baseline level that contains the error. (Search for the error in tests/phpstan/baseline ).
  2. Change the parameters.level in phpstan.neon to the level identified in step 1.
  3. Comment out the includes for that level _and all levels above it_.
  4. Run the following command:
# Replace {%N} with the level identified in step 1
   vendor/bin/phpstan analyse --generate-baseline tests/phpstan/baseline/level-{%N}.php

## Prior Art

This is based off the work done in #853 with some notable differences:

  • Latest trunk and PHPStan (1.12.7) - as of writing.
  • No changes to WordPress core codebase.

This is to limit the scope of the PR and hopefuly prevent it from going stale.

  • The phpstan.neon.dist file wraps the tests/phpstan/base.neon config that holds the codebase configuration (what to scan/skip, etc), with the top-level file holding the "rules".

This makes it easier for contributors to remediate errors by creating a local phpstan.neon file.

  • Removed many of the parameters.ignoreErrors in favor of error baselines (one per each PHPStan level).

See Remediating errors using the error baselines section above.

  • The tests/phpstan/bootstrap.php file has been reorganized to mirror the order that the constants are defined in the WP lifecycle.

This will hopefully make it easier to maintain in the future.

  • Added inline annotations to the various configs.

## Additional Notes

  • PHPStan Level 6 is the highest level that can be baselined without making changes to core code. While, I'd personally recommend baselining at level 8 (with ignoreErrors) - Level 9+ is primarily about mixed types - that can be handled incrementally in a future PR that contains the necessary code remediations.

## Next Steps

  • [ ] Add a GH Workflow to run PHPStan on PRs.
  • [ ] Triage the baselines for good candidates for ignoreErrors.

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

#11 @justlevine
14 months ago

Per @johnbillion 's request (https://github.com/johnbillion/wordpress-develop/pull/4#issuecomment-2429389292), I opened up a new PR against trunk that focuses solely on PHPStan config and doesn't make any code changes.

https://github.com/WordPress/wordpress-develop/pull/7619

My next step is re-reviewing the comment history in https://github.com/WordPress/wordpress-develop/pull/853 to see if there's any additional obvious candidates to be moved from the baselines to ignoreErrors.

PS: I'm part of the current Mentorship Cohort, so any and feedback or direction (even what might seem "obvious" to you) is much appreciated.

Last edited 14 months ago by justlevine (previous) (diff)

#12 @justlevine
13 months ago

I've begun remediating errors surfaced by PHPStan in _individual PRs_ against https://core.trac.wordpress.org/ticket/52217 so they can be reviewed independently of implementing the tooling (and decrease the changes of things going stale or getting polluted with merge conflicts).

Will update this comment periodically with links to the specific PRs to avoid continuously spamming everyone who's just here for the tooling. Too time consuming.

Last edited 11 months ago by justlevine (previous) (diff)

This ticket was mentioned in Slack in #core by justlevine. View the logs.


13 months ago

#14 @justlevine
13 months ago

In https://github.com/WordPress/wordpress-develop/pull/7619/commits/94cf5e494a1f62055746635d16b1b84e8ee66476, I added wp-includes/blocks to the list of excludePaths.

Per https://github.com/WordPress/wordpress-develop/blob/trunk/tools/release/sync-stable-blocks.js, this directory is sourced from WordPress/gutenberg (h/t @thelovekesh 🙇), so we can't remediate them directly.

My understanding is that things are different in Gutenberg land, and as such efforts to introduce PHPStan to Gutenberg code needs to happen separately from this ticket.

This ticket was mentioned in Slack in #core by justlevine. View the logs.


13 months ago

This ticket was mentioned in Slack in #core by justlevine. View the logs.


11 months ago

This ticket was mentioned in Slack in #core by justlevine. View the logs.


11 months ago

#18 @justlevine
8 months ago

Was suggested I post stats whenever I rebase https://github.com/WordPress/wordpress-develop/pull/7619 on trunk, to provide a relative metric of both how much we're remediating and how much continues to slip in without PHPStan.

As of 12 April 2025 (based on https://github.com/WordPress/wordpress-develop/commit/4ce25a5b915ce4409fb6339b8b1ce46fc10aeb0a )

(Previous rebase: 21 Feb 25 https://github.com/WordPress/wordpress-develop/commit/165d803380c561f39caeb0cbdc44f5c06c988cba )

PHPStan Level Error Count New Remediated
0 14 - -
1 65 - 5
2 326 1 2
3 195 5 -
4 985 6 1
5 557 8 5
6 2134 3 0
Last edited 8 months ago by justlevine (previous) (diff)

This ticket was mentioned in Slack in #core by justlevine. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by francina. View the logs.


8 months ago

#22 @justlevine
7 months ago

As of 9 May 2025 (vs 12 Apr 2025)

PHPStan Level Error Count New Remediated
0 9 - 5
1 63 - 2
2 313 0 13
3 195 - -
4 987 5 3
5 557 - -
6 2134 - -

This ticket was mentioned in Slack in #core by justlevine. View the logs.


7 months ago

#24 @johnbillion
7 months ago

  • Milestone changed from Future Release to 6.9

@johnbillion commented on PR #7619:


7 months ago
#25

@justlevine Thanks for all your work on this.

  • I think this is likely to get merged before the bump to PHP 7.4. I know it's a bit extra work but are you interested in switching this to PHPStan 1.12 for compat with PHP 7.2?
  • Do you feel like working on the GitHub Actions workflow to get PHPStan running during CI?

@justlevine commented on PR #7619:


7 months ago
#26

@johnbillion

  • I think this is likely to get merged before the bump to PHP 7.4. I know it's a bit extra work but are you interested in switching this to PHPStan 1.12 for compat with PHP 7.2?

Happy to - I can move the version bump over to my remediations branch (https://github.com/justlevine/wordpress-develop/tree/chore/phpstan/level-0 ) so we can keep remediating core, while this PR runs through the process.

  • Do you feel like working on the GitHub Actions workflow to get PHPStan running during CI?

I can definitely try! Not sure I fully understand the WPCS guidelines for CI/CD (or that reusable-*.yml pattern), but at minimum it'l work and can serve as prior art for someone else to get core-worthy 🤞

@justlevine commented on PR #7619:


7 months ago
#27

@johnbillion - PR's updated.

CI seems to be working (though I guess we won't know for sure if the cache works until it's merged) but I'm not sure why PHPStan itself is failing:

https://github.com/user-attachments/assets/15cef2d7-f237-427b-a2fd-fe107ab907d0

Any ideas?

#28 @justlevine
6 months ago

In addition to dropping back to WP 7.2, the last several commits saw some other fine-tuning of the PHPStan config:

  • missingType.return is now ignored (Level 6), since it's too noisy until we start inlining : void into our function signatures. (Per WPCS we dont @return void).
  • dynamicConstantNames has been given an initial list of constants
  • treatPhpDocTypesAsCertain has been turned back on to remove a bunch of false positives. (While imo this hinders the ability to see remediation opportunities, it is technically the correct config choice).

As a result, and per the latest rebase, the counts are as follows. (New/remediated doesnt make sense so I'm just noting the changes).

Versus 9 May: https://core.trac.wordpress.org/ticket/61175#comment:22

PHPStan Level Error Count Changed since last Notes
0 10 +1
1 34 -29 mainly from treatPHPDocTypesAsCertain:false
2 525 +212 mainly from drop to PHPStan 1.x
3 185 -10
4 264 -723 mix of dynamicConstantNames and treatPHPDocTypesAsCertain:false
5 498 -59
6 108 -2026 mainly ignoring missingType.return


This ticket was mentioned in Slack in #core by justlevine. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-coding-standards by justlevine. View the logs.


6 months ago

This ticket was mentioned in Slack in #core by justlevine. View the logs.


6 months ago

#32 @justlevine
6 months ago

Trunk today versus 31 May: https://core.trac.wordpress.org/ticket/61175#comment:28

PHPStan Level Error Count Changed since last
0 11 +1
1 29 -5
2 514 -11
3 134 -51
4 219 -45
5 498
6 106 -2

There's around ~400 fewer errors on the remediation branch - many due to the application of @phpstan- specific type annotations, some from fixes awaiting cherry-picking / PR review against #63268

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


6 months ago

#34 @westonruter
5 months ago

  • Owner set to justlevine
  • Status changed from new to assigned

#35 @justlevine
5 months ago

PHPStan has been bumped to v2.x to align with the PHP7.4 bump in #62622 cc: @johnbillion

Trunk today vs 18 June:

PHPStan Level Error Count Changed since last
0 13 +2
1 22 -7
2 320 -194
3 164 +30
4 243 +24
5 528 +30
6 106

As you can see, most of it is a distribution from Level 2 to higher levels thanks to improvements to PHPStan, while progress continues to be made independently via #63268

#36 @dmsnell
5 months ago

Thanks all who have worked on this; looks like it’s been in the works for a long time. I am leaving my commentary here both as someone who has spent considerable time shepherding large codebases for quality and also as one who has explored many different automated systems to aid in such a task, also in working with CI/CD pipelines as a potential avenue for enforcing code policy.

I think static analysis is great. I hope that PHPStan does a much better job than current tooling at understanding the code and avoiding false positives. That being said, I have a few thoughts on how to temper adding a new tool to the required workflow.

runs as a required check…Improve contributor experience

As a relatively new committer I find the existing required PHPCS/WPCS checks as the most difficult and deflating part of the contribution experience. This is not because I am resistant to code review and feedback: I love feedback. But I find that all automated quality tooling is prone to unintended side-effects that can deter contribution pace and even code quality. With PHP’s inherent parsing complexity, I feel like the quality of PHP quality tools is particularly prone to these kinds of problems.

I would like to share a risk that requiring a passing PHPStan test will lead to quality compromises as developers make awkward changes to appease the linter instead of focusing on relevant and instructional feedback that a developer might provide. Having seen a fairly consistent drip of this kind of interaction with the existing linting checks, I am worried that adding more will only reinforce the culture of high-effort/low-payback changes that gate-keeps against more contribution and mentoring of folks who earnestly want to learn how to improve the quality of their code and contributions.

To that end, I have observed a number of ways that introducing this kind of tooling can help:

  • Never reject a PR because of linter failures. The linter false-positive rates are high, the errors often unhelpful, and people are generally willing to examine the results if presented helpfully. Reviewers can look at the results of the linting and ask a submitter to address the issues they find important. Reviewers can reject PRs/patches and can do so as a human agent with whom someone can discuss the rejection, but programs leave no room for negotiation or education and routinely gaslight contributors when the linter itself is misunderstanding the code.
  • Spend the extra time up-front to helpfully display linting issues in the Github PR checklist. There are few things more irritating than jumping through multiple pages, opening various hidden panels, and jumping down the page to find that the linter is annoyed that your array definitions aren’t zig-zagged enough, or to find that after going through all this work, you have to do it three more times because the linter only reveals some of its gripes on each invocation.
  • Very clearly indicate where to report bugs in the linter/configuration.
  • If a linter is going to enforce arbitrary styling rules it can perform; move those checks into auto-formatting steps in a git hook. Occasionally the linter catches a missing space, which is not a big deal, but blocking a PR with a red alarm and failing it is demoralizing and insulting, since the computers are fully capable of preventing this situation.

Having lint warnings is incredibly valuable since they can highlight risky code.

Enforcing lint issues though is a task so prone to backfiring. I would love to see us explore a period in which the lint issues are left as an auto-updating comment on PRs where authors and reviewers could discuss them. Doing so makes it easier to turn on higher levels of checking without interrupting contribution, and gives a nice pathway to ramp up contributors’ awareness of the codebase, the style, and quality issues rather than beating them down with fiat gate-keeping.

To repeat the issue of real risk to code quality, I find that when faced with a system that only rejects contributions, people learn to figure out how to get the linter to stop rejecting and lose sight of any intention the linting rules might have had. This leads to regressions, bugs, and awkward code that runs counter to the original goals. This occurs with the existing rules on the WordPress repo. People are human and complicated, which is probably why simplistic and mechanistic solutions require so much care. If someone eagerly makes a poor choice in their patch to appease the linter, then the onus is on the reviewer to notice the even-more-cumbersome code whereas the original issue might have popped out to a reviewer had the contributor not attempted to hide the problem.

Thanks for all of your hard work on this deceptively-difficult problem of moving the contributor base towards higher-quality contributions without discouraging them or accidentally-encouraging dangerous practices.

#37 @johnbillion
5 months ago

@dmsnell I have also had those same concerns, particularly about coding standards which are purely stylistic. I've even given up on PRs to other open source projects where the coding standards rules were far too strict for my liking.

I would love for coding standards checks to not cause a PR to fail, but we'd still need a way for a committer to ensure nothing gets missed when they come to commit the change. It's a bit different with PHPStan though because the rules are about correctness, not just opinion. I'd be hesitant to implement PHPStan but not require its checks to pass, although I'm open to discussing that further.

It's a shame that GitHub doesn't provide a good developer experience for checks that are passing with warnings. A workflow run is either red or green and identifying warnings isn't the easiest.

Automatically fixing PHPCS issues would be the ideal, but AFAIK the WordPress Coding Standards have never been fully compatible with PHPCBF. I don't know enough about PHPCS to know why that is the case.

I think this could move to its own ticket as it's not specific to PHPStan.

#38 @justlevine
5 months ago

For all the lovely folks coming here from the Make/Core proposal here's a quick summary of where things stand:

  1. This ticket proposes adding PHPStan to the development workflow:
    • in an incremental fashion
    • without unnecessary refactoring
    • without unnecessarily disrupting existing WPCS
  1. PR #7619 is a proof of concept demonstrating how it can be done.
    • It proposes a structure for the base config, and using both allowlisted errors + "baselines" of existing errors to make incremental progress.
    • It manually splits the existing baselines of tech-debt by individual PHPStan level, so proposal reviewers can get a better feel of what sort of errors each level addresses, and to help visualize the in-parallel remediation efforts (tracked in #63268).
  1. What's still needed:
    • Determine what PHPStan level to use when scanning new code. (I used level 6 because my goal was visualization, but there's level 7+ errors that can't be baselined)
    • Review the baselines for those levels, and decide if that rule should be ignored for new code or stay in the baseline. (E.g. if there's something that conflicts with existing WPCS).
    • Finalize the config + squash the baseline.
    • Document:
      • using PHPStan (linting, ignoring/remediating, cli)
      • maintaining the PHPStan config (for committers/component maintainers)
      • the process for adopting stricter PHPStan rules/levels in the future
  1. What's beyond the scope:
    • Fixing PHPStan errors in existing code (they are baselined until each one is explicitly ignored or remediated)
    • Updating WPCS to take advantage of PHPStan features and/or changes to our PHPDoc parser to accommodate. (There's no reason to delay the immediate value that PHPStan offers without those features by trying to reach consensus on that can of worms)


#39 @dmsnell
5 months ago

It's a shame that GitHub doesn't provide a good developer experience for checks that are passing with warnings.

Agree with you on this, @johnbillion, and that’s part of why I think the burden should fall on us before we proactively reject work. I’ve seen some very good integrations that rely on interaction via comments, both on the PR front-roll and via line-level comments.

we'd still need a way for a committer to ensure nothing gets missed when they come to commit the change

I wonder how far we could go simply listing these issues. Supposedly we are all already reviewing code, and if there are errors noted on the PR then it would fall within the review obligations to make sure they aren’t overlooked. This feels no different than if you came to one of my PRs and pointed out a subtle type-casting bug in my use of in_array(). Maybe I respond with some explanation of why it looks wrong but isn’t, or maybe I say thanks for catching my mistake before I push it to the world.

Having that interaction to me is part of embracing the humanity of code review. If there’s no way to discuss it then it’s just a gate-keeper. Maybe people prefer this, and I won’t stand in their way; just wanted to share since the technologies and automations we build have real impacts on the people who have no choice but to appease them or find workarounds.

I’m still a bit worried that if we provide only an approve/reject mode without discussion, then people will try to find awkward workarounds before the underlying issues can be identified, and then it becomes that much harder to find and deal with them later.


Has anyone collected data on which changes would have been rejected had we been employing this for the past year or so? It’d be informative to know the breakdown of caught errors that were missed in review against rejected valid work.

#40 @justlevine
5 months ago

@dmsnell if I understand you correctly you seem to be saying that adopting PHPStan as a linting tool means we need auto -accept/-reject/-anything, but I don't think that's the case.

The workflow as built into #7619 is that:

  1. PHPStan/the CI surfaces _new_ errors.
  2. When faced with a PHPStan error, it can be triaged by:
  • fixing the error
  • // @phpstan-ignore rule.type (Reason it's not a valid error) to suppress a false-positive _in file_.
  • Adding it to the "baseline" of existing tech debt. This is like saying "We'll triage and decide how to handle it later"
  • adding it to our ignoreErrors ruleset. This is the equivalent of saying "this entire rule is invalid".

So yeah at no point should this devolve into some automated "approve/reject mode without discussion" 🙏🤞

Has anyone collected data on which changes would have been rejected had we been employing this for the past year or so? It’d be informative to know the breakdown of caught errors that were missed in review against rejected valid work.

So while like I said nothing would be *rejected*, but as for getting a feel for would be flagged as I tried and I guess failed to explain in the message above yours:

  • the baselines to see what actual existing tech-debt is being flagged, and the commit history on those baselines to see how remediation/tweaking the rulesets for WP for those rules are handled. The counts from that are what I've been sharing in this thread, and if you check the instructions on the PR, you'll see instructions on how to comment out a baseline or an error to see the LOC the violation is on as part of a remediation workflow.
  • my remediation branch that I've been cherrypicking from. You can see how e.g. using @phpstan-return conditionals or asserts both narrow down the false positives _and_ improve the code type-safety, or how // @phpstan-ignore is used to handle a false positive in a similar way to PHPCS. And all the individual remediations that I've been cherrypicking onto #63268 (and will hopefully become a blessed task and I can pair with a committer and batch them into larger PRs)
Last edited 5 months ago by justlevine (previous) (diff)

This ticket was mentioned in Slack in #core-coding-standards by justlevine. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by benjamin_zekavica. View the logs.


4 months ago

#43 @dmsnell
3 months ago

@justlevine looking forward to meeting face to face at some point ☺️ thanks for your patience with me as I delayed in responding. some of that is me thinking about the points you raise, and some is me being distracted by other matters.

adopting PHPStan as a linting tool means we need auto -accept/-reject/-anything…When faced with a PHPStan error, it can be triaged by

I’m not saying it means we have to, but I will be surprised if we do this and people don’t implicitly choose to. In fact, suggesting these three “triage” resolutions is part of the system that I think worries me. I’m worried that when the only way to get the CI tests to pass is to take action, then one of two things is going to happen: they will litter WordPress with even more ignore comments; or they will make awkward workarounds (these are both noted above in my previous comments).

If the output is an informative comment in the PR, even if that’s just the first iteration, then it gives people knowledge and they can choose what to do with it without feeling shame or guilt. My views on this are definitely influenced by my experience with WPCS and some other systems like code coverage requirements. I’ve seen it backfire and am definitely at risk of overreacting, but given that we’re discussing adding a CI step to reject changes that we haven’t used broadly yet, I think we can take steps to mitigate the risk.

PHPStan/the CI surfaces _new_ errors

I’m guessing this includes a percentage of changes that touch legacy code and make the legacy code better in some ways without solving all potential problems with the legacy code. It’d be reassuring to know that in practice, we don’t have these false positives, or find out that the linting demands scope-creep and prevents small but not-yet-complete/perfect refactors.

Adding it to the "baseline" of existing tech debt. This is like saying "We'll triage and decide how to handle it later"

Those baselines are kind of gnarly. I appreciate how they are stored out-of-band instead of in noisy comments that cloud the actual logic in the code. What happens though when PHPStan updates and adds some new validation which flags a bunch of existing code? Does updating the PHPStan version imply we need to potentially add hundreds of new items to the baseline?

Does PHPStan report on which baseline ignores end up matching no lines of code? It would be great if exclude rules which never trigger could be automatically removed. In fact, it’d be lovely if we have counts of how many times each exclusion rule is triggered — maybe that’s already available.


My goals here are purely to ask about the systematic effects of introducing this new demand and to ask if could spend a little more effort up-front to change how the detected issues are reported; please accept these in the most charitable tone because in no way do I want to be dominating the choices here, and I’m grateful for folks like you figuring out how to automate this kind of tooling.

I’m still most curious about running the proposed rules against every commit from the past few years and see how many issues would have been raised in those commits had we been running this code. If we end up with hundreds of new issues, that could clue us in to revise some rules before we deploy it. If we end up with no new issues, well maybe that indicates that our review process is already pretty good and a non-rejecting comment could be helpful without the risks (another way to explore this would be to examine each existing rule violation, and examine hold old that rule violation has been there, and then to look at the distribution. have we introduced new rule violations at a steady pace, or have we gotten much better in our process and stopped adding new violations, or were we better in the distant past and are now introducing issues at a more rapid pace).

And absolutely I’m a huge fan of surfacing the report as a comment that is automatically updated when the tool runs again. That saves people the hassle of making multiple navigations on an ever-slower Github interface and removes the stigma for people trying to contribute by not rejecting their work with a big red X. It’s more work to do that, but not much more. It doesn’t reject invalid work, but it also doesn’t reject valid work.

Thanks to all for your continued work and discussion on this.

This ticket was mentioned in Slack in #core by justlevine. View the logs.


3 months ago

#45 @justlevine
3 months ago

looking forward @dmsnell - one of these days I hope to finally make it to a WordCamp (if only I'd known I was choosing pheonix mid-August over portland 🌞)

adopting PHPStan as a linting tool means we need auto -accept/-reject/-anything…When faced with a PHPStan error, it can be triaged by

I’m not saying it means we have to, but I will be surprised if we do this and people don’t implicitly choose to. [...] My views on this are definitely influenced by my experience with WPCS and some other systems like code coverage requirements. I’ve seen it backfire and am definitely at risk of overreacting, but given that we’re discussing adding a CI step to reject changes that we haven’t used broadly yet, I think we can take steps to mitigate the risk.

I think we're in agreement (and I don't think it's an overreaction). Between learning from the experience of adopting W(/PH)PCS and some of the particulars *of* PHPStan (IIRC their concept of a central "baseline" and other config choices is also in response to similar unintended side-effects, I feel pretty confident that between docs and maybe some changes to how those CI communications those changes we can ensure that there's no reason/incentive to e.g. write code for the tool, and that it reduces friction for contributors/committers instead of adding to it.


PHPStan/the CI surfaces _new_ errors

I’m guessing this includes a percentage of changes that touch legacy code and make the legacy code better in some ways without solving all potential problems.

Indeed it does. Solely illustrative example: say, we've got a legacy class that's using a PHP8 breaking is_callable( [ $my_class_name::class,'non_static_method'] ) 5 times, and someone updates code to add a new one.

PHPStan lets us:

  • ignore the file entirely (without touching the code)
  • ignore the specific error type (or message, or type + message combo) for that file (without touching...)
  • baseline the 5 existing occurrences (or combo baseline/ignore if there was justification? idk an intentional shim maybe?), but still show an error about the 6th (without touching...)
  • manually baseline *or* ignore the 6th error, independently of the how the existing 5 are handled (by updating the config but still w.o the code).
  • and _then_ the usual ignore/disable line/rule exit hatch's that should only be a last resort i.e. the rare "true" false-positive.

All independently of any other PHPStan errors found in that legacy class (like how $my_class_name isn't defined anywhere because the old code uses $my_CLASs_Name for "legacy compatibility" ).

True, the IRL success depends on our ability to accurately translate WPCS into PHPStan rulesets (and where the proposed PR needs the most help) but since we can adopt it piecemeal (via config/baselines) and iterate on it without disrupting the codebase, the risk here is really low and easy to rectify if we don't get it perfectly tuned in the initial commit.

Those baselines are kind of gnarly.

Yupppp and thank you, I've been splitting them by-level manually for the sake of review (instructions in the PR description) since as noted seeing what sort of errors are caught is key to help us align the rules for WPCS so we're not needlessly bothered by false positives. If people like that approach I can bash-script it, but most projects treat it like any other auto-generatable (but *diffable*) tool file e.g. package-lock.json`, since once committed you only care to see if anything is being unintentionally added.

What happens though when PHPStan updates

PHPStan follows a strict SemVer definition, where changes to flags/rules/smells only happen in major releases, where the upgrade path would be to update the relevant configs + autoregenerate the baselines. (Theoretically possible they'd choose to rename code-annotations the way PHPUnit did, but considering they're meant as a last result even if there wasn't a b/c migration path the disruptions would be minimal)

In minor/patches it's possible that a bug fix or improvement previously hidden errors to be detected or remediated, (e.g. now that it recognizes the array shape, it stops warning you to check if the array_key_exists() before accessing and instead that $arg['permission'] is really $args['has_permissions'] and expects a callable<bool> instead of true|\WP_Error ).

Assumedly we'd pin a specific version number at merge to avoid that happening unintentionally.

Does PHPStan report on which baseline ignores end up matching no lines of code?

Indeed, there's the [reportUnmatchedIgnoredErrors config value](https://phpstan.org/user-guide/ignoring-errors#reporting-unused-ignores) that lets us decide whether unmatched ignored errors should be treated as an error itself, including the granular counts/file where the error was previously.

Currently, the unmatched baseline rules *does* error, but mainly way mainly so I_could easily track and illustrate for review the tech debt changes on the PR (and remediation branch), but I've been hesitant whether that's the right approach for a core commit, since even though tech debt remediation should be intentional/auditable if that's not the goal of the PR, we don't want it getting the way (to avoid new tech debt slipping in by an autogenerate, writing code to appease the tool, lockfile-style merge conflicts, etc etc)...

I *was* leaning towards recommending that regenerating the baseline as a new 1-command step in the release process, but:

It would be great if exclude rules which never trigger could be automatically removed.

Riffing off this, we could regenerate-baselines with CI on merge (in theory, I don't fully get the svn <=> gh mechanics). A core commit means everything there has been reviewed/approved and therefore justifies a new baseline of "existing tech debt". But that means increased potential for merge conflicts, albeit a trivial one 🤔


I’m still most curious about running the proposed rules against every commit from the past few years and see how many issues would have been raised in those commits had we been running this code.

Like minds 😇

Lazy version alternative is the history at https://github.com/WordPress/wordpress-develop/pull/7619/commits.

Each of those tests: regenerate PHPStan baselines after rebase commits is the snapshot of changes from PRs merged into core E.g. https://github.com/WordPress/wordpress-develop/pull/7619/commits/9c8f09656b9bbf2f4f7173328074c75cf2d63581

(rebasing loses the commit date fidelity of when specifically over the last year did the resync happen, I've been averaging a rebase ~3-4 weeks).

Mind you, the results are a bit juiced with the in-parallel remediations merged via #52217 #63268 , but in a way I think that kinda proves the hypothesis here: it's "so" nondisruptive that parallel development, active remediation, even major config changes can happen in complete, and the diff is still just a single (or split by level current) autogenerated file from a single --generate-baseline command


@dmsnell if you have the time I'd love to connect and walk you through the specific baselines/errors/behaviors of in the current PR, probably the most efficient way to tweak the setup so it's not just theoretically/philosophically sound but practically robust IRL.

(Offer's open to any other contributor/committer looking to give feedback! Slack DM's open)

This ticket was mentioned in Slack in #core by francina. View the logs.


3 months ago

#47 @SirLouen
2 months ago

I'm having troubles to analyse wp-includes/user.php. I've tried removing that file and removing all the baselines ignores that target specifically that file, and it goes through the analysis without troubles.

It's just this file that is causing, (at least in my repo), some sort of conflict with the latest trunk

Is anyone experiencing this trouble?

Last edited 2 months ago by SirLouen (previous) (diff)

#48 @SirLouen
2 months ago

I've been able to hunt down the problem to wp_insert_user function.
This function is what is causing the timeout.

#49 @SirLouen
2 months ago

  • Keywords changes-requested added

Also, I was wondering, shouldn't we add tests/phpunit to excludePaths > analyse in base.neon ?

This ticket was mentioned in Slack in #core by welcher. View the logs.


8 weeks ago

#51 @welcher
8 weeks ago

  • Milestone changed from 6.9 to Future Release

We're about 1 week from 6.9 Beta 1 and this seems to be still in discussion/requesting changes so I am going to punt it. If it can be finished before the cut off, please feel free to move it back and do so.

#52 @justlevine
6 weeks ago

I've been able to hunt down the problem to wp_insert_user function.
This function is what is causing the timeout.

More specifically it's the $userdata['user_pass'] = '' assignment that was added in https://core.trac.wordpress.org/ticket/22114 . I've added the file to the excludePaths.analyse for now.

Also, I was wondering, shouldn't we add tests/phpunit to excludePaths > analyse in base.neon ?

No need, since we only need to excludePaths that are included by paths, so only items inside src need to be excluded.

---

Syncing with trunk and some other tweaks gives us a current count of:

PHPStan Level Error Count Changed since last Remediation branch vs trunk
0 26 +13 28 +2
1 21 -1 14 -7
2 315 -5 276 -39
3 159 -5 109 -50
4 236 -7 193 -43
5 519 -9 425 -94
6 113 +7 118 +5

This ticket was mentioned in PR #10419 on WordPress/wordpress-develop by @justlevine.


6 weeks ago
#53

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

This PR adds a PHPStan configuration for PHPStan level 0, along with tests and docs.

Based from #7619 - which remains in use to explore adopting future levels (alongside parallel remediation branches).

Proposal: https://make.wordpress.org/core/2025/07/11/proposal-phpstan-in-the-wordpress-core-development-workflow/

Note: See TracTickets for help on using tickets.