Problem/Motivation

Thanks to #3579177: Enable GitLab CI and fix pipelines we're now running phpstan on our codebase. However, we're still using the default level of 0, which is only catching the bare minimum of problems. It'd be nice to get to a higher level and see how much that can help us both cleanup our existing code and to prevent new MRs from introducing regressions.

I'm also curious about this since I was confused that in D9 there's a deprecation about extra arguments to test assertion methods like statusCodeEquals(), which I was assuming would start throwing exceptions in D10 and up (as the deprecation message said), but that doesn't actually happen. When I asked why, I was pointed to #3322961: Fix WebAssert since PHPStan can now check the number of arguments passed to methods. So it seems that at least core is relying on being at a higher PHPStan level to catch problems like this.

Steps to reproduce

Proposed resolution

Bump the PHPStan level. See what happens. Fix what we can, baseline the rest. Repeat.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork forward-3579288

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dww created an issue. See original summary.

dww opened merge request !12

dww’s picture

dww
we/he/they
commented
Title: Explore bumping the PHPStan level » Bump PHPStan to level 5 and fix errors
Status: Active » Needs review

Nice. Locally I got us up to level 5 with fairly minimal changes, and some of them definitely helped catch some weirdness.

Bumping to level 6 starts to get really noisy, 125 new errors, basically requiring that all params and return values are typed, which is more of a change than I'm in the mood for, potentially BC breaking, and perhaps requiring a new major version. No thanks. 😅

But IMHO this is already a big improvement, so I'd be happy to merge this.

p.s. I put another argument at the end of one of those statusCodeEquals() calls in PermissionsTest and lo, it would now be flagged:

 ------ ----------------------------------------------------------------------------------------
  Line   modules/contrib/forward/tests/src/Functional/PermissionsTest.php
 ------ ----------------------------------------------------------------------------------------
  28     Method Behat\Mink\WebAssert::statusCodeEquals() invoked with 2 parameters, 1 required.
         🪪  arguments.count
 ------ ----------------------------------------------------------------------------------------

Huzzah.

  • dww committed 2e3f7e67 on 4.0.x 
    task: #3579288 Bump PHPStan to level 5 and fix errors

dww closed merge request !12

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

I don't see any reason not to do this. Merged.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.