[wp-trac] [WordPress Trac] #61175: Integrate PHPStan into the core development workflow

[WordPress Trac]

#61175: Integrate PHPStan into the core development workflow
--------------------------------------+-------------------------
 Reporter:  westonruter               |       Owner:  justlevine
     Type:  enhancement               |      Status:  assigned
 Priority:  normal                    |   Milestone:  6.9
Component:  General                   |     Version:
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+-------------------------

Comment (by justlevine):

 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 *diff`able*) 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)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/61175#comment:45>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform