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

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.

Enable MissingFunctionReturnTypehintRule and MissingMethodReturnTypehintRule phpstan rules

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

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

Just brainstorming to help keep the idea moving.

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?

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

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.

This needs another re-roll, I'm +1. Pinged the other committers to try to get sign-offs.

Discussed this briefly with @xjm, @longwave, @larowlan and @quietone - we're all +1 so I am removing the RM/FM tags.

@xjm suggested doing a core blog post announcing the change since potentially a lot of existing MRs in the queue would start failing on PHPStan (even if they hopefully would have been eventually marked needs work via manual review). I'm not sure this is necessary because this is already a coding standard, we're just adding it to phpstan.

Double checked and this is already clearly documented in the coding standards exactly how we'll be enforcing it in PHPStan https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s...

It needs docs update and blog post is a good idea as well

I think it could be done after commit and will bring nice amount of rerolls for Barcelona's sprints

Although this does go against the PHPStan philosophy which is "Don't pick and choose which rules you apply, it works best if you apply all of them", at this point I'm happy to have any progress towards better type support in Drupal.

The only thing I'd want to add is that this doesn't change the need for the related issues mentioned in the parent. Even though it provides an important step in the right direction by adding return type hints for new code.

Also unrelated to the issue at hand but important to share with everyone in this thread vis-a-vis "how to add return types to interfaces": PHPStan is perfectly happy accepting a return type as PHPDoc as per the following example.

This means that as long as we force the rule to be enabled and downstream projects enable it then a viable strategy is:

1. Ensure all interfaces have a properly typed `@return` type annotation
2. In a major version promote the return type from PHPDoc to PHPCode.

PHPStan will already start reporting the return type mis-match based on the PHPDoc in downstream projects and help them implement it on classes (because as mentioned earlier in this thread, a class may always return a narrower type than what it extends/implements thanks to covariance). That means that as long as they fix those PHPStan errors before making the major jump, the major version jump won't actually be breaking for them. This aligns well with our current "Fix all deprecation on the last minor and then jump to the next major without changes" philosophy.

The (minor) risk I see with this is that people start adding diverging types to class methods that inherit a common, yet untyped method interface. Covariance would allow do so now, but then it will make life more complicated later once we will try to typehint the interface.

See #3470913: Ensure getWidth()/getHeigth always return ?int and Symfony's upstream [ErrorHandler] Add support for @return-type-will-change #58134 for a different (but certainly longer) approach.

The (minor) risk I see with this is that people start adding diverging types to class methods that inherit a common, yet untyped method interface. Covariance would allow do so now, but then it will make life more complicated later once we will try to typehint the interface.

I think PHPStan can actually really help us here, in the way I described. I think that's something the Drupal Update Bot may be able to do even. However, it requires that we update coding guidelines that projects that consume Drupal should, before they upgrade a Drupal major version:

• Ensure they're on Level 3 (or above) OR enable at least PHPStan\Rules\Methods\MethodSignatureRule
• AND ensure the parameter reportMaybesInMethodSignatures: true is configured

If we consider changing the PHP return type of an interface as a breaking change and reserve it for majors (12.0.0), i.e.

interface Foo {
  // Before
  public function bar();
  // After, this would be a breaking change.
  public function bar() : string;
}

However, we ensure that we fix the PHPDocs correctly:

interface Foo {

  // Before
  /**
   * @return
   *   The return type.
   */
  public function bar();
  // After, this is not a breaking change and PHPStan starts flagging it downstream.
  /**
   * @return string
   *   The return type.
   */
  public function bar();
}

The above code would flag an implementation issue given the configuration described at the start: https://phpstan.org/r/f54ce223-907b-49b2-a45e-70e817347b13

Needs another rebase.

As per #26

Rebased and rebaselined. Will probably need to do again before commit, so here's the steps I took.

git reset --hard 96204beed3f
git rebase 11.x
composer install
vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/.phpstan-baseline.php
git add core/.phpstan-baseline.php
git commit -m "Update baseline"
git push --force

Pushed! All I did was regenerate baseline.

Back to NR after rebase.

Note; when you have done a composer install in core directory, things will fail ;p

And lol steven ;x

Don't want to step on toes can we revert the change to the README

There is a change to README.md that is out of scope, seems your test commit for maintainer permissions ended up here ;)

@smustgrave can you force push again?

I just noticed also, my 11.x was polluted by a small mini commit. I used the steps in the issue instead of my normal rebase workflow.

And that children, is why you always rebase on remote branches.

*removed*

Done, think we are good now!

Let's do it. Committed/pushed to 11.x, thanks!

Removing 'needs documentation updates' because this is already policy, the only new thing is the phpstan enforcement of it.

Awesome!

This is causing problems when new code adds an existing trait, where the trait's methods don't have return types. e.g. #3390193: Add a drupalGet() method to KernelTestBase comment #38.

Similarly in #3452852: Add create() factory method with autowired parameters to PluginBase we are adding a new trait but can't add a return type because the trait is implemented in the base class and existing subclasses may already be overriding the method.

For these cases we just have to add to the baseline, I don't see a way around it.

Another instance of the problem mentioned above: https://git.drupalcode.org/issue/drupal-3425081/-/jobs/3144156 (from #3425081: Integrate Navigation with Workspaces )

Amending attribution.