Increase PHPStan level to 8 with strict rules

[julienfalque]

This PR sets PHPStan analyse level to the maximum and enables strict rules. This results in 7905 new errors on 2.15 branch and we clearly don't have the time to fix all right now so I created a baseline file.

Nothing should be added to this file ever, except when merging the PR up to higher branches, where more errors might be reported. In such case, the baseline file should be regenerated. Then we can fix the errors step by step when we can. In the meantime, new errors in future PRs will be reported.

[kubawerlos]

It looks crazy, wouldn't be a nightmare during merges?

Why this approach instead of step-by-step?

[julienfalque]

Fair point. This approach allows to enable all checks we want for future code without having to fix all existing code first. But I don't think it will report more than a few errors when merging PRs that are not up-to-date with upstream or when merging patches up to higher branches.

Maybe regenerating the baseline file for each branch after merging this PR will be easier than updating it manually. I can take care of that if we agree on merging this.

[SpacePossum]

The baseline way seems good to me. @julienfalque can you merge this and than merge upstream making sure on the branches the baseline file is good?
Side note, do we want to do this with a psalm baseline as well?

[julienfalque]

I'll merge this one 👍

Regarding Psalm: I have no experience with it and I intended to use this project as a playground to test it. But if anyone wants to do it, I'm fine with that :)

[julienfalque]

I'm marking this PR as WIP because I want to check if some errors from the baseline file should be in the regular file instead (errors yet to be fixed vs. errors we actually don't want to fix).

[julienfalque]

Half of the errors from the baseline are missing return types. A lot of them would be : void. On PHP 5.6 this can be fixed by using @return void PHPDoc tag instead but our current code style forbids that so let's ignore those for now.

Also, let's drop PHP 5.6 support please.

[kubawerlos]

I'm still kind of pessimistic with having this huge baseline, I'm afraid after it is merged we would start to think these issues are solved.

Why not increase the level step-by-step and add errors to ignore list grouped by type, like the one with return void?

[julienfalque]

Because ignoring "by type" is not ideal. It hides all errors of the same kind, but maybe some of them can be fixed while some other can't (e.g. for BC reasons), so we have to check the errors one by one, which is very time consuming. Doing that would take months.

On the other hand, the purpose of the baseline file is to list the errors that have yet to be fixed, we should never consider them fixed. And once merged, contributions to fix remaining errors will be easier.

The has no return type specified exception I added is because we can't fix most of them until we drop PHP 5.6 support. We'll have to remember to remove the exception then and fix the errors properly.

[staabm]

I am a friend of a high phpstan level and the baseline approach. did things like that in some of our projects already.

I would recommend to a lower level though, because level 8 is very strict and might even introduce rules you dont agree with.

we usually try to fix errors on level 0, 1, ,2 step by step (as you did) and then do the rest by baselining arround level 5.

IMO 8 seems to be too high for now.

to get an idea what which level achieves you might consult https://phpstan.org/user-guide/rule-levels

[julienfalque]

IMO we do want level 8 and strict rules here. As far as I remember, rules like e.g. forbidding implicit boolean casts in control structures are things we (often) check during code review anyway.

[staabm]

I see. if your goal is 8 you should go for it.

have a look whether running with so many warnings/ignores will affect your phpstan runtime, otherwise 👍

[SpacePossum]

Looks good to me, having the baseline will prevent new issues from being introduced 👍
After this we can start cleaning out the baseline.

[SpacePossum]

thanks @julienfalque

[ktomk]

CONTRIBUTING.md does not mention how to run phpstan on a development box nor that it is a requirement at all albeit it makes the build fail (ref #4919).

Hidden dependency?

[julienfalque]

See #4976.