Bump PHPStan to level 9 and accept a large baseline

It will simply make impossible to contribute new code, since you'll have to develop huge workarounds to get sorted all the info which is missing from core codebase at the moment (description of iterable types, narrowing of types, etc etc).

I'd want to respectfully disagree. I definitely think there's a learning curve. We'll have to go through that as a community at some point, but if we do our lives will become easier. Better documentation on how to solve common type issues can help there and I'm willing to help write that if it can help speed up Drupal's move to strict-typing.

One habit that I see with developers is the impulse to add /** @var */ annotations to solve typing warnings when upstream types are not yet set. That is indeed something that becomes harder to maintain. A much better way of solving that is to use assert instead. It has the benefit of ensuring that your tests fail if what you're saying is false and PHPStan will let you know when the upstream types have improved and the assert is no longer needed.

If iterable types is truly a problem then we can disable that behaviour specifically with checkMissingIterableValueType: false but having the iterable types specified makes your foreach loops so much nicer and gives you type completion without having to do extra type annotations or asserts within your loop.

An average contrib module can get to L5 relatively simple, L6+ is more work in satisfying PHPStan than in anything else.

As a counter-example, I would consider the Open Social distribution a non-trivial codebase with about 69 top-level modules (and a whole bunch of nested modules). We've been running at level 8 for three years and counting and our code has become significantly higher quality with a measurable reduction in bugs as a result.

I worry here that just jumping straight to level 9 will mean contributors have more work to do when submitting MRs against existing code [...] In turn that means having to make decisions whether or not to explicitly add more things to the baseline.

I think that's a fair worry, specifically because they'll have to think about types in a way that we don't have to do now though. It's a trade-off of short-term pain for long term gain. The work they'd have to do now would otherwise be work we'd have to do at some point in the future.

The baseline is also very good at ensuring that these kinds of changes can happen locally without having to go and fix the whole file just because you're changing a single line.

sometimes we have to do things for backward compatibility reasons and I think satisfying PHPStan is not always possible without breaking BC?

I'd be curious to see examples of this and whether I can offer ways around it! In general PHPStan allows a lot of ways to add type information without actually changing the functioning of the code (e.g. through assert or @annotations).

What might happen is that Drupal becomes better at typing its code which will cause downstream projects to suddenly see PHPStan errors where there were none before. However, I personally don't consider that a breaking change because it's clarifying intention. As long as the PHP code keeps working without errors and doing what it promises then that would still be backwards compatible.

I'm skipping #8 for now until we know what the baseline will actually look like. At that point we can decide whether it needs to be excluded from generated archives. I think it's a good proposal and an easy addition though.

Replying to #10, #11, and #12, paraphrased:

What about merge conflicts in the baseline? They're tedious

I think there's two things I could bring up to hopefully alleviate this concern.

Firstly, I think the decision to use PHP for the baseline rather than NEON will help git reduce the number of conflicts. The NEON baseline is basically a giant list of YAML which I believe git treats as plaintext which often causes it to not know which array item marker (-) belongs to which rule. Example excerpt from Open Social below.

parameters:
	ignoreErrors:
		-
			message: "#^Method Drupal\\\\activity_basics\\\\Plugin\\\\ActivityAction\\\\CreateActivityAction\\:\\:create\\(\\) has no return type specified\\.$#"
			count: 1
			path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php

		-
			message: "#^Method Drupal\\\\activity_basics\\\\Plugin\\\\ActivityAction\\\\CreateActivityAction\\:\\:create\\(\\) has parameter \\$entity with no type specified\\.$#"
			count: 1
			path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php

		-
			message: "#^\\\\Drupal calls should be avoided in classes, use dependency injection instead$#"
			count: 1
			path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php

For PHP there are more clearly defined boundaries because the start and end of a rule are distinctly delineated by different tokens (the PHP open and close array syntax). An excerpt from the new PHP baseline from Drupal Core:

<?php declare(strict_types = 1);

$ignoreErrors = [];
$ignoreErrors[] = [
	'message' => '#^Call to function method_exists\\(\\) with \'Composer\\\\\\\\Composer\' and \'getVersion\' will always evaluate to true\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/../composer/Composer.php',
];
$ignoreErrors[] = [
	'message' => '#^Method Drupal\\\\Composer\\\\Composer\\:\\:drupalVersionBranch\\(\\) should return string but returns string\\|null\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/../composer/Composer.php',
];
$ignoreErrors[] = [
	'message' => '#^Parameter \\#2 \\$base_dir of method Drupal\\\\Composer\\\\Generator\\\\ComponentGenerator\\:\\:generate\\(\\) expects string, string\\|false given\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/../composer/Composer.php',
];

This should help git better differentiate between different changes and reduce the number of merge conflicts that are actually flagged. It should also help developers better understand conflicts because they're dealing with PHP code, rather than the middle of a huge YAML list.

Secondly, however, if developers are manually fixing conflicts in the baseline file then I think we're doing a disservice to them. The baseline is the result of the changes in our code and is a file that's automatically generated by PHPStan so we really shouldn't be fixing conflicts manually. My recommendation here would be to educate developers to fix conflicts by regenerating the baseline file (during a rebase or merge commit):

git checkout <base-branch> -- phpstan-baseline.php
vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline core/phpstan-baseline.php 

PHPStan Baseline reviews are also pretty straightforward to know whether such a regeneration was successful. Any removals are improvements in code-type quality and will be caught by the code review itself, so they can be blindly accepted. Any additions need some attention and hopefully spark discussion for code improvements.

When we do end up going there, I'd want us to consider bringing in

phpstan/phpstan-strict-rules

I'm a big fan of the strict rules, but I think there's value in the higher PHPStan levels even without it. So I would propose we evaluate it in a follow-up issue :)

And at least having configs like:

reportUnmatchedIgnoredErrors: true
checkMissingIterableValueType: false
checkGenericClassInNonGenericObjectType: false

I can happily say that reportUnmatchedIgnoredErrors is enabled in PHPStan by default, so there's no need for us to add it to the config unless we want to turn it off (which we shouldn't).

For checkMissingIterableValueType I already had a short discussion with mglaman on Slack which I think is worth repeating here:

PHPStan tells us the number of errors in the baseline with the new level 9 settings is 98503. Of those, 19447 contain the sentence "no value type specified in iterable type array" which brings our error count down to 79056. About 372 further errors contain missing "in iterable type" in a non-array types. I still think there is value in fixing those non-array type iterables (think about things like field item lists) and thinking about how we introduce other iterable types. So my proposal would be to manually ignore errors of the type *in iterable type array* then we can still fix other iterable types but at least don't need to go out and fix all the render arrays that are being pushed around immediately.

Disabling checkGenericClassInNonGenericObjectType would remove another roughly ~500 ignored errors. Though I would argue that use of generic typing in Drupal might increase (as we add @template annotation to indicate a class is generic over a certain type) and having that information available can make consuming such code a lot easier (because it ensures the return types don't need to be mixed). So I'd urge keeping the generic check enabled. Disabling it would for example negate almost all benefits of the types for #3425201: Implement a Result type in Drupal Core.

On Slack alex.skrypnyk mentioned we probably want to have a core committer review this issue. Given the impact this issue has on the requirements for future code to be merged and reviewed, I agree. I believe the release manager tag is most appropriate (and they can escalate to "Needs committer feedback" if needed (since it's mentioned as use sparingly).

I've split out the change from Neon to PHP into #3426548: Convert the PHPStan baseline from NEON to PHP

That also includes the update to the GitLab CI to output the baseline in the PHP format there. The functionality to output the baseline as artifact was previously implemented in #3403653: Incorporate improvements to how contrib runs PHPStan to core

This branch has been rebased on top of 11.x and the work from #3426548: Convert the PHPStan baseline from NEON to PHP has been cherry-picked in to see the pipeline status in this one. That means this issue is now postponed on the other one :)

I wonder if we can run this in CI against files in `git diff --diff-filter=A --name-only` i.e. require level 9 for all new files but avoid the conflict hassles of the baseline?

The maintainer of PHPStan wrote a blogpost about why we shouldn't do that :D

It also makes intuitive sense because if I change the type of an interface then that's the only file that's changed. However, we know that changes in interfaces might affect all classes that implement them, so we can't rely on only analysing the interface.

I made a small script (attached as txt file) that allows us to use the PHP baseline to get some insight into what type of errors we're dealing with, which can be a bit more precise than my previous estimates. I still believe that we should embrace the baseline to make sure we don't increase the count of errors we're introducing in new code (and a lower PHPStan level simply isn't going to do that).

However, I am open to seeing if we can get better insight into the contents of the baseline and see if we can resolve some issues using PHPDoc annotations in parallel. I'd also be willing to invest some time in that if that helps acceptance of a baseline size and this issue landing sooner.

Here's the output of the current iteration of the script. I'd love to know if there's a preferred way of tackling these (e.g. I could add all return types to Drupal's functions as I expect them to be relatively straightforward, but that might be a patch with 2000 changed lines.

Analysing 59257 ignored PHPStan errors
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
.................................

2026 functions without a return type specified.
533 methods on interfaces without a return type specified.
19064 methods on other classes without a return type specified.
169 unreachable pieces of code.
106 if-statements that will always be TRUE.
3 if-statements that will always be FALSE.
59 partial conditions always TRUE/FALSE.
346 undefined property accesses on an object.
642 undefined property access on other things.

36309 ignored errors without classification.

There was a thread in Slack about the issue raised in #24

I think there needs to be a conversation about adding a 15 megabyte file to the core directory and at the very least decent instructions about how to not deploy this to your site.

drumm mentioned "Composer does no archiving, its just automating downloading zip files from from GitHub & ftp.drupal.org, and git clone git.drupalcode.org & elsewhere". That means that the proposed solution by Mglaman of adding the following to .gitattributes under core/ would ensure the file is not distributed through composer or Drupal.org archives.

/.gitattributes export-ignore
/.phpstan-baseline.php

This does still let the baseline be downloaded when people install with composer using the --prefer-source flag which I would say is proper behaviour since the baseline is part of the drupal/core source.

Adjusted the script to gain a bit more insights at level 9 :)

The lines that say "always" or "unnecessary" is where PHPStan allows us to remove code :D
Operations on "may be NULL" or "may be FALSE" are code-paths that may manifest in bugs.

Analysing 59257 ignored PHPStan errors
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
.................................

2026 functions without a return type specified.
533 methods on interfaces without a return type specified.
19064 methods on other classes without a return type specified.
169 unreachable pieces of code.
106 if-statements that will always be TRUE.
3 if-statements that will always be FALSE.
59 partial conditions always TRUE/FALSE.
346 undefined property accesses on an object.
642 undefined property accesses on other things.
4085 method calls on values that may be NULL.
413 method calls on values that may be FALSE.
1032 property access on values that may be NULL.
186 property access on values that may be FALSE.
1478 values returned that don't match the return type.
427 invalid types supplied to foreach.
69 binary operations resulting in error.
3644 calls to undefined method (usually value is not of specific enough type).
2260 unsupported operation on mixed type.
21 calls to undefined function/method.
72 calls to assert which isn't needed.
11 calls to is_array which are always FALSE.
33 calls to is_array which are always TRUE.
2 unnecessary calls to is_bool.
2 unnecessary calls to is_callable.
2 unnecessary calls to is_int.
13 unnecessary calls to is_null.
5 unnecessary calls to is_numeric.
4 unnecessary calls to is_object.
1 unnecessary calls to is_resource.
30 unnecessary calls to is_string.
3 unnecessary calls to is_subclass_of.
47 unnecessary calls to method_exists.
5 unnecessary calls to property_exists.
570 unnecessary PHPUnit Assert.
1285 functions with missing parameter type.
4967 method with missing parameter type.
229 other statements are always TRUE.
388 other statements are always FALSE.
654 drupalLogin calls may receive FALSE.
6964 incorrect type of parameter provided for method call.

7407 ignored errors without classification.

I don't think modifying the baseline further or moving it out of the repository is a good idea. It would make the development experience of running PHPStan locally (and updating the baseline) too cumbersome.

While I do understand that 15 MB is a lot on the scale that Drupal is operating, we should also place the size of the file in perspective. I think our primary goal should be that people who deploy Drupal to production shouldn't suffer its cost. For that we have a good proposal which I've committed to the branch (although it does need testing which I can not do against a dev branch in a fork). I believe that as long as the file is not bundled in the archives on Drupal.org or in the dist version of Composer that's on Packagist then we've achieved that goal.

It's unrealistic to try and save the same 15MB when dealing with the raw source repositories (e.g. a git clone or --prefer-source composer install) because those are likely scenarios in which you want to have the baseline present. For an initial clone of drupal/drupal the new baseline file would represent a 7% increase in repo size; for an inital clone of drupal/core the new baseline file would represent an 8% increase in repo size (counting the existing baseline as 0-size in both cases).

However that size would only be downloaded once, since it's a text file that git can efficiently update rather than having to re-download it on every pull request.

Additionally we know that this is a file where our aim is to make it smaller quickly (because we'd be addressing real issues and improving the developer experience of anyone using Drupal) and it's not a file that will stay this big indefinitely.

Fresh git clones of both drupal/drupal and drupal/core to show the size of the repo's inital clone for comparison to the increase caused by the baseline.

$ git clone git@git.drupal.org:project/drupal.git
Cloning into 'drupal'...
remote: Enumerating objects: 967424, done.
remote: Counting objects: 100% (12915/12915), done.
remote: Compressing objects: 100% (2246/2246), done.
remote: Total 967424 (delta 11040), reused 10823 (delta 10660), pack-reused 954509
Receiving objects: 100% (967424/967424), 230.15 MiB | 4.31 MiB/s, done.
Resolving deltas: 100% (699506/699506), done.
Updating files: 100% (17353/17353), done.

$ git clone git@github.com:drupal/core.git
Cloning into 'core'...
remote: Enumerating objects: 767457, done.
remote: Counting objects: 100% (19190/19190), done.
remote: Compressing objects: 100% (8354/8354), done.
remote: Total 767457 (delta 10741), reused 18556 (delta 10149), pack-reused 748267
Receiving objects: 100% (767457/767457), 186.46 MiB | 13.95 MiB/s, done.
Resolving deltas: 100% (542285/542285), done.
Updating files: 100% (17264/17264), done.

The other issue landed! Marking this as "Needs work" because it needs a rebase.

In #3426548-21: Convert the PHPStan baseline from NEON to PHP mstrelan mentioned

Since this file is php do IDEs like PhpStorm try to analyse it? It can obviously be excluded but does that involve manual steps?

This needs some more investigation to impact of the large file. My initial response was:

I can not answer for all IDEs but only for PHPStorm. PHPStorm does seem to index the file. As far as I found there's nothing we can do against this with things we ship in the repo. I also don't know if it's a problem (i.e. in what ways it affects PHPStorm usage).

There are settings in PHPStorm that allow excluding files which are global settings that apply to all projects. So in case the analysis of the file by PHPStorm causes problems for someone it's a one-time fix.

longwave's feedback from #33 was previously addressed in the commit between #30 and #31 assuming the packaging works how we thought it did on Slack.

I did some local experimentation with PHPStorm by opening it on the 11.x branch. This allows PHPStorm to settle on 2,23GB of memory used according to the activity monitor. I then switched to the 3426047-bump-phpstan-to-level-9 branch for this issue. The indexing time that PHPStorm needed was near instant (basically a progress bar flash) and memory changed to 2.63GB. I'm not sure how scientific this is and whether all 0.4GB of memory pressure can be attributed to the baseline or whether PHPStorm does other things when switching branches that require memory.

I added ".phpstan-baseline.php" to "Exclude files" under "Settings > Directories" and retried the experiment. I opened PHPStorm on the 11.x branch which settled at 2.25 GB. This time no re-indexing seemed to be triggered and memory usage changed to 2.36GB when checking out the branch for this issue.

Rebased the MR on top of the 47 new commits on the 11.x branch. Also added the baseline file to CSpell as requested in #37 and #40.

I ran the same analysis script I attached in #29 to give an indication of how many new errors (according to the higher rule level) were added in those 47 commits. I've attached the analysis output and manually added (+x) increase, (-x) decrease, or (~) equal indicators for each rule.

Analysing 59851 (+594) ignored PHPStan errors
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
................................................................................
..................................

2028 (+2) functions without a return type specified.
533 (~) methods on interfaces without a return type specified.
19091 (+27) methods on other classes without a return type specified.
171 (+2) unreachable pieces of code.
104 (-2) if-statements that will always be TRUE.
3 (~) if-statements that will always be FALSE.
61 (+2) partial conditions always TRUE/FALSE.
301 (-45) undefined property accesses on an object.
642 (~) undefined property accesses on other things.
4112 (+27) method calls on values that may be NULL.
414 (+1) method calls on values that may be FALSE.
1029 (-3) property access on values that may be NULL.
186 (~) property access on values that may be FALSE.
1561 (+83) values returned that don't match the return type.
436 (+9) invalid types supplied to foreach.
69 (~) binary operations resulting in error.
3648 (+4) calls to undefined method (usually value is not of specific enough type).
2339 (+79) unsupported operation on mixed type.
19 (-2) calls to undefined function/method.
72 (~) calls to assert which isn't needed.
11 (~) calls to is_array which are always FALSE.
33 (~) calls to is_array which are always TRUE.
2 (~) unnecessary calls to is_bool.
2 (~) unnecessary calls to is_callable.
2 (~) unnecessary calls to is_int.
13 (~) unnecessary calls to is_null.
5 (~) unnecessary calls to is_numeric.
4 (~) unnecessary calls to is_object.
1 (~) unnecessary calls to is_resource.
30 (~) unnecessary calls to is_string.
3 (~) unnecessary calls to is_subclass_of.
48 (+1) unnecessary calls to method_exists.
5 (~) unnecessary calls to property_exists.
574 (+4) unnecessary PHPUnit Assert.
1287 (+2) functions with missing parameter type.
5016 (+49) method with missing parameter type.
229 (~) other statements are always TRUE.
392 (+4) other statements are always FALSE.
656 (+2) drupalLogin calls may receive FALSE.
7222 (+257) incorrect type of parameter provided for method call.

7497 (+90) ignored errors without classification.

From #39

Actually, the needs review bot and open MR should tell us how often there are conflicts or other issues, right? For that concern.

I'm not entirely sure. A merge conflict occurs when two parallel changes touch the same lines of code and git is not able to determine how to reconcile those. This MR makes + 295436/− 1079 changes to the baseline (since we include all currently known possible errors), so it essentially touches every line in the existing baseline and any other change will have a conflict with it once.

The normal number of changes in the baseline are much lower than 300k lines. With that in mind, the chance of two MRs touching the same lines of code in the large baseline go down significantly.

From #36

This is true for the _current_ file, but cloning with no depth limit will still pull in all the prior revisions. Not really gonna change anything, but unless we rewrite history or people routinely clone without depth limit (could be an idea for CI if it isn't already) then this is "here to stay" once it happens.

Unless I've missed something the only open concern (besides the potential for merge conflicts) is that because of the way git works (it captures history) the increase to the repository size with the new baseline size would be permanent for the lifetime of the project. This would affect clones which do not limit depth (by default clones fetch all history depth); we have taken steps to ensure this doesn't affect production deployments which don't use git.

I don't have a good solution for this and with access to 4G/5G and fiber in all my workplaces I think I might be slightly too privileged to have a proper say in this matter. Git has improved support for very large repos (measured in Gigabytes, so much larger than where Drupal is at) but these do require steps taken on the consumer side and are not something we can do from the repository side of things.

Thanks for the input everyone! And thanks alexpott and phenoxproxima for sharing your experiences with level 9 in the recipe fork.

Responding to the recipe example

If I look at the commits it's a bit difficult for me to see where exactly the friction is coming from (don't get me wrong, I do see the friction). There's two points I think I could summarize it as (and please correct me if I'm wrong): firstly a learning curve, secondly Drupal's use of arrays.

The learning curve is very understandable and I do think it's something we'll have to guide the Drupal community through. We're essentially going from a very loosely "do what you want" language to a very strictly typed language. Adding @var annotations or preferably assert statements (since they fail your test if your assumption is wrong) is a part of the journey we'll have to take. Regardless of our strategy with PHPStan levels, that's something we'll have to go through at some point. The good news is that PHPStan will tell us if asserts are wrong (and thanks to Drupal using Bleeding Edge will also tell us if @var statements are likely wrong). That provides us with a way to fix issues at the call site ("I know this is type X") while allowing us to fix the origin of the bug (the actual type annotation) and then have PHPStan tell us which call-sites no longer need the asserts/annotations.

The second thing is Drupal's use of arrays. This is the only thing that ended up in the baseline. It's probably the hardest thing to fix in Drupal regardless of levels because it'll require re-thinking how we pass around our render data and define a more well-defined structure for it. This is exactly why this issue proposes a singular exception to the strict rules of PHPStan (from the issue summary):

Exceptions added
In order to not frustrate Drupal's use of render arrays and instead allow people to slowly start adding types to those we ignore the error ... has no value type specified in iterable type array. We do not disable the type requirement for all iterable types because specifying them does have a lot of value outside of render arrays in improving the type-safety of loops in code.

We do not disable generics altogether because there's a lot of places (like Drupal's Entity API which is now partially patched by PHPStan Drupal) where generics can really add value in telling downstream code what type of value exists within a container. Introducing generics without immediately validating them with PHPStan is likely to cause us even greater pain in the future, hence why they're only disable for arrays and we keep them for other classes (where assertions/@var annotations can more easily be written if needed).

Why running PHPStan on a lower level is a fallacy

It's a bit challenging that PHPStan called its configuration steps "levels" because they make people think of them like "levels in a video game" where if you beat one level, the next one becomes easier. However, that's not really the case for PHPStan. I personally see them more as "groupings of complexity" (although depending on your code-base some higher levels might be easier than lower levels).

The challenge with Drupal's current approach is that while we're slowly fixing some issues at the lower levels, we're steadily introducing new things to fix at higher levels which are just sitting there waiting. The value across the levels also isn't equal. For example we all agree that type-hints are incredibly useful but these aren't checked until level 6 (that's the same level that introduces the generics that many people stumble over). It's not until level 8 that PHPStan will help us out in preventing "calling some method on NULL" which are errors that, if they slip through tests, will case a white screen of death for a site. The only difference between level 8 and 9 is PHPStan being strict about "mixed" which is a good thing because without that rule people might see "mixed" as a solution over finding the more specific union they're probably actually interested in. PHP's mixed is akin to TypeScript's any, which the TypeScript community has long since learned is dangerous. To remedy that they've introduced unknown which changes any from "could by anything" to "you must figure out what this is", that's basically all PHPStan's level 9 does over level 8.

Some statistics

To back-up my claim that even though we're slowly chipping away at lower levels, we're introducing issues at higher PHPStan levels at a higher rate that we'll want to fix in some future, I've analysed ~2100 commits from 10.1.x until 11.x (around the end of March) for Level 1 and Level 9 of PHPStan and plotted their difference. This is based on the analysis script that I shared in this issue earlier but with all errors grouped into a specific category/explanation.

The scripts used to do this as well as the raw result data can be found in https://github.com/Kingdutch/analyse-drupal-phpstan

As we can see we're slowly chipping away at the 600 errors that are in our current baseline. Meanwhile we're introducing about 2 errors per commit on average, which dwarfs the work that we're doing at lower levels as we climb from about 55000 errors to nearly 60000 errrors.

This is to me what spurs my motivation to try and change our approach to how we tackle code quality since I believe the benefits that preventing the errors shown by PHPStan can bring us truly outweight the effort needed to prevent them.

In an attempt to make discussing the different levels of PHPStan based on data rather than my own or others' feelings I've analysed commit ec28f25d1e on the 11.x branch at level 1 through 9 and ran the results through the analysis script. The resulted categorisation is listed in a table below to show which errors and how many of them there are for the different levels of PHPStan.

Based on the table above I would love to hear the value people assign to the different categorised errors and whether it's worth preventing them. For example I value preventing null-based bugs because I've seen them as errors a lot and similarly believe that "mixed" is not a good type to use in 90% of circumstances. However if people disagree with me that might give us a more data and decision driven approach to finding which level we should land on going forward. Additionally if that's not 9 it may give us some ideas of what criteria we want to adhere to to move to higher levels in the future and how to ensure the higher levels don't grow as we fix lower level issues.

If we did accept a larger baseline and bump the level, do you think it would make sense to then take a horizontal approach instead of a vertical approach to reducing the baseline size? e.g. Working by rows in your table, instead of by columns (which is what we're doing now).

It does feel like a lot of those rows would be easy wins but also with a high return. There are definitely some rows where reward vs effort vs disruption would definitely be difficult.

Yeah I think that would absolutely be the way to go. I do think there's a challenge that they may not be simple find/replace, so I'm not sure how large we can make batches. For example we may introduce a return type in the PHPDoc of an interface and then fix our classes to adhere to them (where we can introduce it in PHP). That in turn may cause PHPStan to say "Hey these type assertions you're making here are no longer needed" and we can remove those. So there's connection between the different rules.

The benefit is that the higher level of PHPStan can give us the confidence that if the types are correct at a local level (e.g. we make sure to accept string|\Stringable) they'll also help us out on the project level. That should be a big help to reviewers for these kinds of issues even if they're not copy/paste.

Or, another idea - add a level 9 job to core's CI pipeline, but allow it to fail. That will at least allow us to know if we're introducing new errors, whilst not blocking on preexisting problems.

I saw the suggestion and like it in theory but I'm struggling with a viable way of making it work. One of the differences with PHPStan vs tools like CSpell/PHPCS is that we always want to run it on the entire codebase because changes in one file can result in behaviour changes in any other file. So with that in mind I think this route would still require us to commit and update the baseline in case of errors, but there'd be a discussion of which errors are then acceptable.

If we don't commit (or update) the baseline then it's going to be really difficult to say what part of the code caused the new errors and what errors were introduced in previous commits. I think that probably leads to more frustration.

The only way I can come up with to do this and not commit the baseline would be to generate the baseline from the base commit of the PR and then run it against HEAD of the PR, so only the change in errors would be reported over which reviewers could make a decision of whether it's acceptable or not. So basically run PHPStan twice in the CI. The downside of this is that it does make people reliant on the CI for their PHPStan work and does not allow to easily perform these checks locally (unless they also run it twice locally). A composer install between runs might be needed because updated dependencies can affect type information.