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

 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
 [https://github.com/WordPress/wordpress-
 develop/commit/ba4709cdc2756faf694ba42d3920ea2642e724ca 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.

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