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

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

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

catch → credited larowlan → .

catch → credited longwave → .

catch → credited xjm → .

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).

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.

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 📌 Ensure getWidth()/getHeigth always return ?int Needs review 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'll redo it.

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.

catch → closed merge request !8763

Awesome!

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

Similarly in ✨ Add an Autowire trait for plugins Needs review I am 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.

We could add @phpstan-return void annotations to the methods in the traits (or an actual return type if it's safe to do so)

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

Automatically closed - issue fixed for 2 weeks with no activity.