Enforce return types in all new methods and functions

Stats for baseline looks big but looks like it should be better then growing amount of newcode

+ 75718
− 1390

MR appears to be unmergable

Obviously it will need constant rebasing. Setting back to NR to get actual review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Let's just review the idea and the changes to phpstan.neon.dist. If we're happy with that then we can regenerate the baseline. We can also review what's currently being added to the baseline, even if it's not mergeable, to verify that we're only identifier: missingType.return.

Enable MissingFunctionReturnTypehintRule and MissingMethodReturnTypehintRule phpstan rules

What if we broke these out into 2 tickets would that be easier?

$ grep "Method" 8763.diff | wc -l
10753
$ grep "Function" 8763.diff | wc -l
7335

Not sure there's any difference reviewing 10000 or 17000 changes.

Would it be easier to breakout into groups of components or group of modules?

Just brainstorming to help keep the idea moving.

I appreciate that the MR is massive but really the only change that needs review at all is to phpstan.neon.dist. Everything else is proven by the phpstan job passing. I don't think committing in chunks will help, this probably just needs "Needs ____ review" but I don't know what ____ is, maybe FM?

Framework would probably be best. And would just ping them.

Can you add the current size of the baseline and the size with this MR?

Rebased, re-baselined and rewrote the issue summary. FWIW I don't know why some lines were removed from the baseline, but that can only be a good thing as long as the phpstan job still passes.

So what would be the scope or game plan to address these eventually? So people aren’t doing a bunch of tickets

This issue is not about adding the return types to existing methods, it's just about preventing new methods from being added that don't have a return type. I think adding return types to methods in run time code needs BC consideration most of the time.

Big +1 on this and agree with #12. The longer we wait to do this the larger the potential baseline as more and more code gets added, I know we're pretty good at asking for return types but this would enforce it.

*jumps on the +1 bandwagon*

I think adding return types to methods in run time code needs BC consideration most of the time.

Having tried to work on PHPStan lvl 1 issues in the current baseline in a previous life, I very much think this is the case.

I see a slight risk of people adding a new missingType.return error to the baseline, since they're seeing the current ones also being in the baseline, but with a explanatory CR and, since almost nobody will read that, a vigilant NR and core committers crew (so no problem there :) we will be fine.

Let’s see if it can get in

Also imagine if this gets in a number of tickets may get sent back

This is a great step imo to start enforcing.

This might also need an update to the Coding Standards. And if this is enforced do we need to say something about new classes that might not be able to add since they for example implement an interface or extend a base class? There might be place where this is imossoble.

Due to covariance you can add a return type to a child class when the parent or interface doesn't have one. See https://www.php.net/manual/en/language.oop5.variance.php