Bump PHPStan rule level to 2

We'd probably need to add Jan0707/phpstan-prophecy as a dependency to interpret the prophesized methods that are yielding a lot of L2 errors.

The title is at odds with 📌 [meta] Fix as many PHPStan level 2 issues as possible before bumping the rule level Active

#4 yes, it's the contrary... :)

Alternative approach.

mondrake → changed the visibility of the branch 3425412-bump-phpstan-rule to hidden.

mondrake → changed the visibility of the branch 3425412-bump-phpstan-rule to active.

Adds 7785 new level-2 errors to the baseline.

Requires --dev jangregor/phpstan-prophecy dev-master. Release 1.0.0 has a failure that was fixed by https://github.com/Jan0707/phpstan-prophecy/pull/316. We would need to wait for a point release here.

NR for the concept.

Tagging.

I generated a baseline for each level from 1-6 so see where is the biggest jump.

level 1: 630 errors
level 2: 8415 errors
level 3: 10657 errors
level 4: 14012 errors
level 5: 16015 errors
level 6: 64641 errors

I think if we're happy with a baseline of ~8000 errors at level then we should aim a little higher, perhaps level 3, or, if there is appetite, then 4 or 5.

TBH I do not think we can be happy with a baseline of 8k. But the pre-work to reduce it is nowhere.

I am rather hoping that bumping the level 1 notch up will drive more fixing, and in any case prevent further non-L2-compliant code to be introduced.

We have introduced L0 2 years ago with 1k errors more or less, and bumped to L1 in Nov 22 - still having 630 errors now is not a great score... bumping to 8k is a very large step, and more than enough for some time to come.

Somewhere I read L2 should be the decent minimum for any codebase, let's walk there before we run further.

Let me also say there's another reason not to skip levels: each level up will require contributors some time to adapt their coding style - use of assertions or inline doctypes to clarify type of variables, later on description of iterative types etc... it takes time to digest.

I remember contributing to Doctrine DBAL (that is on L9), without prior experience of PHPStan reports... at first I simply could not understand what I was doing wrong... then I joined the pieces and started seeing the benefits, but you really need to have a codebase that is already at that level to be able to contribute meaningfully. In the current shape of Drupal codebase, jumping ahead would mean that new contribution will have to do all sort of workarounds to satisfy PHPStan, whereas it should be PHPStan helping us improve the code.

Was hoping for some low hanging fruit we can knock off to quickly reduce the number but it's not looking that promising:

$ grep message core/.phpstan-baseline.php|sort|uniq -c|sort -rn|head
    203 	'message' => '#^Access to an undefined property Drupal\\\\Core\\\\Session\\\\AccountInterface\\:\\:\\$passRaw\\.$#',
    201 	'message' => '#^Access to an undefined property Drupal\\\\Core\\\\Session\\\\AccountInterface\\:\\:\\$name\\.$#',
     85 	'message' => '#^Call to an undefined method Drupal\\\\Tests\\\\WebAssert\\:\\:waitForElementVisible\\(\\)\\.$#',
     80 	'message' => '#^Call to an undefined method Drupal\\\\Tests\\\\WebAssert\\:\\:waitForElement\\(\\)\\.$#',
     79 	'message' => '#^Call to an undefined method Drupal\\\\Tests\\\\WebAssert\\:\\:assertWaitOnAjaxRequest\\(\\)\\.$#',
     68 	'message' => '#^Call to an undefined method Drupal\\\\Core\\\\TypedData\\\\TraversableTypedDataInterface\\:\\:get\\(\\)\\.$#',
     48 	'message' => '#^Call to an undefined method Drupal\\\\Core\\\\Entity\\\\EntityInterface\\:\\:get\\(\\)\\.$#',
     41 	'message' => '#^Call to an undefined method Drupal\\\\workflows\\\\WorkflowTypeInterface\\:\\:addEntityTypeAndBundle\\(\\)\\.$#',
     32 	'message' => '#^Call to an undefined method Drupal\\\\Core\\\\Entity\\\\EntityInterface\\:\\:getTranslation\\(\\)\\.$#',
     30 	'message' => '#^Cannot access property \\$uri on object\\|false\\.$#',

Even solving all those will only take 867 off the 8,000+ errors.

MR appears to be unmergable, surprised the bot never picked it up.

I'm not super into the phpstan stuff like others here in this ticket but in regards to bumping levels. When a new rule or anything is added usually involves having to go back and update several MRs. Can be argued that's just the name of the game with Drupal but can understand community frustration when they have an MR all ready and it keeps getting sent back because of changing rules. So think doing small increments is good.

Please let’s focus on opinions if this should go or not, rather than starting NR-NW tennis. The MR will become unmergeable every day, but it’s not rerolling it that the question changes.

Also please keep comments to the scope of this issue: bump to level 2. There’s another issue that preaches level 9, lets have the appropriate level discussion there, in one place.

Thanks

Just pointing out that phpstan-prophecy 1.0.1 has been released and the MR has been updated to that version, so the concerns in #8 about the dev release are no longer applicable. Let's get this in.

@mstrelan thanks a lot for clarifying in #17 that phpstan-prophecy is no longer blocking

Looks like when we bump to PHPStan L2 we get an initial addition of ~7800 suppressions.
Seeing the amount of active issues to tackle the L1 suppressions, I think it's safe to assume these won't go away quickly.

So the benefit would be preventing new L2 issues creeping in new/refactored code.

It's probably my lack of Google skills, but what are the extra checks in L2 compared to L1, what will be prevented from creeping in by us doing PHPStan L++?

Level 2 gives you:

unknown methods checked on all expressions (not just $this), validating PHPDocs

See https://phpstan.org/user-guide/rule-levels

Or more specifically see https://github.com/phpstan/phpstan-src/blob/1.11.x/conf/config.level2.neon

Thanks @mstrelan, should have found that myself, but somehow didn't. (Makes mental note to check caffeine-percentage of new bought coffee-brand).

Seeing those checks and also the fact that if we want to do it with the least amount of disruptions to other MRs, it's probably now or when D12 arrives, I'm pulling the trigger and put the ball in core commiters court: RTBC for me.

I'm +1 here for the obvious reason that it will stop us introducing new errors, so even if it takes years to reduce the current baseline, at least it will be visibly getting smaller instead of silently getting larger.

On going to L3 or L5 etc., staggering the MR breakage for new rules seems like a good reason to do one at a time. We could still try to go up a level every 6 months or 3 months if we want to (I'm not saying we should definitely do that, but it's worth thinking about). It does look like level 5 would be a couple of small jumps.

I don't want to change the scope, will almost instantly open a follow up if this goes in, but level 3 gives us:

return types, types assigned to properties

On all new code, which most code reviews are picking up anyway, but would be much nicer if it was for free.

One argument against staggering would be that raising the PHPStan level would be disruptive to (almost) all open MRs for each bump, so bumping more then one level would make it less disruptive.

That might be the case when the baseline was in the NEON format, and IIRC was one (of many) reasons the Bump-to-L9 issue is stalling.
But as part of that issue, we switched the baseline over to PHP with the reasoning that Git would be much smarter about merging it.

Is bumping a PHPStan level (with each one bringing in loads of changes on the baseline file) still as disruptive to other MRs as we (or at least I) think, or is this a non-issue by now?

If we're in non-issue land on that, I'm all about staggering, if not, we might want to prevent all/a lot of open MRs grinding to a rebase-halt each time we stagger-bump and do it in one go?

One argument against staggering would be that raising the PHPStan level would be disruptive to (almost) all open MRs for each bump, so bumping more then one level would make it less disruptive.

This is true to an extent, but it will only be disruptive in the following cases:
1. The MR is changing the baseline already, and has a conflict.
2. The MR is introducing new PHPStan failures which are caught by the new level.

Hopefully this won't be almost all open MRs but just a small subset.

There have been 698 commits to the 11.x branch in the past 90 days, so even if we went up a level every three months, anything committed in the interim between bumps by definition won't get affected. That still leaves the thousands of open MRs, but not all of these are rebased that often too.

Just noticed that phpstan-prophecy specifies this in their composer.json

            "conflict": {
                "phpspec/prophecy": "<1.7.0 || >=2.0.0",
                "phpunit/phpunit": "<6.0.0 || >=10.0.0"
            },

So this ATM would block the PHPUnit bump to 10.

We probably want to wait for a release upstream that is compatible?

Actually https://github.com/Jan0707/phpstan-prophecy/pull/334 allowing PHPUnit 10 & 11 was already merged (yesterday!), so it's a matter of waiting for a new point release.

Does this need a dependency evaluation also?

Added a dependency evaluation

phpstan-prophecy just released 1.0.2 that is PHPUnit 10 compatible, bumping

Back to NR?

I refrained from self-RTBC, but it should be a quick check to do it.

It will certainly require frequent rebasing.

Does this still need framework manager review? Otherwise RTBC from me.

Let's bump it to RTBC and see if the core committers think we still need FM approval and if so, if they can ping them :)

We probably need a change record :D

Added two CRs.

Thanks for creating the CRs. This looks good. Baseline may need to be regenerated when committing.

@alexpott was concerned about the size of the baseline, a handful of quick fixes in MR!7547 cut it down by about 25%.

a handful of quick fixes in MR!7547 cut it down by about 25%.

Great, but not sure this is part of this issue? Or even what an acceptable amount of added errors to a baseline would be?

Personally, (n=1, yadayada) this seems like follow-up issue material to me and might be derailing the original intent?
But what do I know? _shrug_

Oh yeah, that's why I made a separate MR, only as a demo. But maybe we move that to a separate issue and try and pick off these and any other widespread ones caused by traits or base classes, before doing this, if there are concerns about baseline size.

Let's do what #42 suggests, it's not just the actual baseline size, but that'll also help to avoid some MR conflicts potentially later on.

Added inline comments, even though it's for the separate issue now I think.