Use PHP 8 constructor property promotion for existing code

Interesting. #3217699: Convert select query extenders to backend-overrideable services was an early attempt to introduce this. It was even committed for some time. Too early, then.

🤩 Yes please!

In #3302755-53: On-the-fly JavaScript minification I wrongly thought this was a bug, I A) didn't know we were doing this, B) had never before seen the new syntax in real code!

So much less boilerplate!

@param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
* The entity type manager.

Just ouf of curiosity. Don't you think this annotation is absolutely redundant for parameters that have native PHP type hints?

Here is another conversation about this.

RE: autowiring: I tried to adopt that a year ago in #3103682-20: CDN module 4.x: no new features + drop BC layers + drop now irrelevant headers + PHP >=8.1 + Drupal >=9.4, but that special comment syntax thing is new I think? Looking forward to trying it in any case :)

Related: #3049525: Enable service autowiring by adding interface aliases to core service definitions.

10.1 is open

Re #6 and multiline syntax, I agree, its much more readable

AFAICT #1014086: Stampedes and cold cache performance issues with css/js aggregation was the first commit to land in Drupal 10 that used this 😊

Bumping priority and tagging to increase visibility; this will have a huge impact on DX. See the (justified!) complaint of just a few hours ago: https://twitter.com/shadcn/status/1576868763932106752

Thanks for the bump on this @Wim Leers.

Agree with #4 re. the constructor docblock, this feels redundant:

• Most of the comments end up as "The X service" or "The X manager" anyway.
• Context can inferred from the type hint and/or is one-click away in an IDE

Actually, https://git.drupalcode.org/project/drupal/-/commit/d0f7178 came first (8 months ago), but was eventually reverted.

One of the reasons of the revert was #3217699-46: Convert select query extenders to backend-overrideable services, and I do not have seen consensus anywhere yet.

Also, it's not in the coding standard AFAICT - which is Catch 22 I think, since the coding standards process seems to be idle at the moment.

All in all, I would welcome this very much, but cannot see clear guidance yet.

I also think we should drop type hinted parameter documentation from constructors, however because that's enforced by coder, we'll likely need a coding standards issue and then a patch to coder to remove it. @quietone has re-started the coding standards group so it might actually be possible to make changes again there - but it'd have to go through the steps...

As mentioned we've already started using property promotion for new code. So I think the main thing here is whether we want to drop parameter documentation at the same time as we apply constructor promotion to old code so we can make all the changes at once, if so we probably need to postpone this one on a coding standards issue to do that, if not we could go ahead with the initial change to property promotion here and do the param docs separately.

I'd like to indicate that the issue with reduntant dockblocks is not specific to constructors with promoted properties.
Most constructors already have typehinted arguments. Promoting their properties makes no difference.
And besides constructors there are lots of ordinary methods, functions, class properties that already have native typehints.

What would we do if a constructor argument uses snake case and the property uses camel case? Do we need to deprecate the old argument in case folks are using named arguments?

Also since this would be a very large MR/patch should this be split up by module/component?

Named arguments we don't need to support, I think we discussed that already in a different issue a while back and just decided if someone is relying on them, it's up to them to keep up.

We already use camel case for properties, so they wouldn't change, just the snake case constructor arguments, so that ought to be OK I would think.

There is an existing rector rule ClassPropertyAssignToConstructorPromotionRector but I'm not sure it handles the mismatched snake/camel case.

Nice! Sorry I missed #14 where you had already identified that.

Is the issue postponed on a coding standard? If yes, which coding standard issue?

@pasqualle property promotion itself we can do without a coding standards issue, and we can already do this for new code.

The bit that needs coding standards changes is removing @param documentation on constructors. Not sure if we have an issue for that yet.

Based on discussions in #2209735: [policy] Remove DI verbosity by removing unnecessary doc lines and #2140961: Allow constructor methods to omit docblocks it seems the constructor doc block should be fully removed.

Re #6
Here is a well explained rationale for this.
https://stitcher.io/blog/why-curly-brackets-go-on-new-lines

#24. The closest Coding Standards issue is #2209735: [policy] Remove DI verbosity by removing unnecessary doc lines. That looks close enough that it can be used for this. Although, I'd like another opinion on that.

I looked at Drupal api for the class mentioned early that already implements this is core, CssCollectionOptimizerLazy.php. It appears that the file is pretty much ignored and there is nothing of value there for the reader. Even the 'view source' link doesn't work for me. I presume an issues is needed in the API project. Based on that I think this needs to be Postponed until that is fixed.

@quietone #2209735: [policy] Remove DI verbosity by removing unnecessary doc lines looks like the right issue for removing constructor param phpdoc.

I looked at Drupal api for the class mentioned early that already implements this is core, CssCollectionOptimizerLazy.php. It appears that the file is pretty much ignored and there is nothing of value there for the reader. Even the 'view source' link doesn't work for me. I presume an issues is needed in the API project. Based on that I think this needs to be Postponed until that is fixed.

This seems to be more generalised on api.drupal.org and not specific to constructor property promotion, I've opened #3342815: Classes missing from Drupal 10 since I couldn't find an existing issue.

Does anyone have the rector config and cli command handy to paste into this issue?

@kim.pepper I was able to get this working with the following config file and commands.

rector.php

<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->paths([
        __DIR__ . '/composer',
        __DIR__ . '/core',
    ]);
    $rectorConfig->rule(ClassPropertyAssignToConstructorPromotionRector::class);
    $rectorConfig->parallel(seconds: 300, maxNumberOfProcess: 4);

composer require --dev rector/rector
./vendor/bin/rector process

Note I had to remove mglaman/phpstan-drupal as it was throwing an error, and had to twiddle the parallel configuration so as not to destroy my laptop.

I assume we can configure code style changes like multi-line parameters.

It seems this only worked for native types, but in #21 @longwave had it working for CsrfTokenGenerator. Was there any additional configuration needed for that to work?

A big +1 for type declarations on class properties is that it really helps avoid abuse in the context of unsafe deserialisation / gadget chains.

See for example #3379512: Harden Drupal against PHP Gadget Chain Drupal9/RCE1.

There's an MR here now and this is unblocked.

This was a bit gnarly to rebase so forgive me if I mangled something.

Because we no longer have all the boilerplate, adding bc for a constructor has to add a lot of it back.

Probably needs its own dedicated issue, discovered in #3354211: Tidy up URL generation for asset aggregates.

For example if you want to remove a constructor argument from somewhere in the middle of the parameters, you would need to do something like this:

1. Define all of the protected properties from the argument being removed onwards.
2. Undo constructor property promotion from the argument being removed onwards.
3. Add the usual deprecation dance in the constructor (this bit is not new).

Then when removing the deprecation, you'd need to undo all of steps 1-3 and go back to constructor property promotion again.

This turns adding bc for a constructor change from a small amount of work to a large amount of work.

That means I think we might need to pair this with #3218030: [meta] Always use named arguments when creating a class instance. See also the discussion in #3183180: [policy, no patch] Document named arguments BC policy.

i.e. we'd make an exception for constructors, recommend calling them with named arguments both from ::create() functions and subclasses (and everywhere else), and then convert core to use named arguments in ::create() functions. And then stop doing the bc dance for constructors when arguments are re-ordered or removed because named arguments should handle it.

But this adds a yet further problem that we're going to be changing the argument names here to match the property names, so if modules started to do that before this issue lands, those modules would break when the argument names change. So really we can only recommend this once versions of core before this change lands are out of support, which would when 10.3 is released/10.1 goes EOL assuming this issue lands before 10.2. I guess we could have a transition period where we do the very verbose workaround, maybe only for classes that contrib is extending or something?

😳🫠

A technically enforceable solution is impossible, only guidance to do these things in the right sequence would help. Or in fact, it'd almost need to be not a sequence but an atomic change?

If every constructor in core (and contrib) would make these two changes in a single commit:

1. adopt constructor property promotion, but keep constructor argument order!
2. adopt named arguments for calling that constructor for all its own calling code

And only then in a subsequent major version:

1. deprecate constructor arguments

… then we'd be in a good place, right? What if we did a PSA for encouraging ecosystem-wide adoption of this pattern?

Is it feasible to detect the use of named arguments and when they're not used, throw a deprecation notice? 🤔 Or would we only be able to do that using something like PHPStan?

My 2 hunches definitely did not work:

<?php

class Something {
    public function __construct(protected string $a, public int $b) {
        var_dump(debug_backtrace(limit: 1));
        var_dump(func_get_args());
    }
}

$s_ordered = new Something('yo', 1337);
var_dump($s_ordered);

print "---------------\n";

$s_named = new Something(b: 1337, a: 'yo');
var_dump($s_named);

— output at https://3v4l.org/17UTb

AFAICT this would only be possible to do through PHPStan 😞

VERY good point!

Should we postpone this issue on that one and other auto-wiring issues? 🤔

How would union types look when there are 4 params and you want to deprecate the second one? Wouldn't you need a union type for params 2, 3 and 4? And if 4 then becomes nullable, when the bc layer is removed there may be confusion as to whether param 4 should remain nullable.

In fact I think 45 was only talking about replacing a param with another one, but often the param may be removed entirely.

@longwave oh nice! I did think about union types, but I didn't think about overwriting the property in the constructor and that's not bad at all. We'd have to drop readonly as well as using union types, but that's fine (https://3v4l.org/X6PNN), can just add it back when we remove the bc layer.

@mstrelan yes pretty much, but that's what it would look like now without property promotion - still have to add union types all the way along if you remove the second of five parameters.

@Wim no I don't think we need to postpone, the union types + overwriting makes this no worse than what we currently have to do for bc, which is more than good enough.

When the last parameter is unused, we can omit it, and use use func_get_arg() - this is already used in some places in core so not really a change from now:

https://3v4l.org/WVIp1

We do the union types deprecation thing already. See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

I wonder if we can provide a trait that will reassign all the property values after the deprecated param using Reflection. This is pretty ugly but as a proof of concept it seems to work:

https://3v4l.org/kWEm7

Edit: simplified a bit:

https://3v4l.org/MtHNJ

Another issue to consider with union types and constructor property promotion is that beyond the constructor the properties are still union types, so any other method can assign the wrong value. In most cases this is unlikely to be exploited, but it will likely cause phpstan to complain that calls to members on those properties may not exist as it can't be certain what type the properties are, so we would have to provide a typehint or assertion to coerce it to agree.

#53 is very clever and worth opening a new issue for.

#54 was about to say this is a pre-existing issue with how we do constructor deprecations, although I wonder if phpstan is able to use the @var on the property definition, in which case maybe it's not a pre-existing issue at all :/

With existing deprecations the property definitions are at least correctly typed, or at least can be correct, since the union is only required for the param.

Opened #3382742: Provide a helper trait for removing deprecated constructor params for #53.

All the blockers are done here.

Clarifying the title that this is now about converting existing code over.

I've updated the issue summary now that constructor docblocks are optional. That should hopefully reflect where we want to get to.

@catch in #49 you're about readonly to apply for the properties, is it separate issue?

@andypost if it's easy I think we can bundle in readonly here, will save changing things multiple times.