[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 dmsnell):

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

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