Use PHP 8 constructor property promotion for existing code

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 🐛 Classes missing from Drupal 10 Fixed 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?

You need this config to make it change untyped protected or public properties:

  $rectorConfig->ruleWithConfiguration(ClassPropertyAssignToConstructorPromotionRector::class, [
    ClassPropertyAssignToConstructorPromotionRector::INLINE_PUBLIC => TRUE,
  ]);

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 📌 Harden Drupal against PHP Gadget Chain Drupal9/RCE1 Closed: won't fix .

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

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

Custom commands failed.

Instead of trying to rebase the existing branch, I think it might be simpler to have a rector script in a patch or branch, and then upload the output from that as a patch; others can confirm that the script output is the same and the bot will happily test the new patch each time HEAD moves on.

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 📌 Tidy up URL generation for asset aggregates Needs work .

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 📌 [meta] Always use named arguments when creating a class instance Active . See also the discussion in 🌱 [policy, no patch] Document named arguments BC policy Fixed .

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 😞

Re #43 can't we just use union types? https://3v4l.org/CLnhZ looks to work.

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.

4 parameters A, B, C, D and we want to deprecate B: https://3v4l.org/08cBi

It seems pretty clear to me that by renaming the nullable argument to $unused that we don't need it after deprecation cleanups.

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

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 📌 Provide a helper trait for removing deprecated constructor params Active 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.

The best time to make these sorts of changes is in the window between beta and RC when we are just polishing and fixing stability issues, perhaps we can do this in the upcoming 10.3.x beta window given we are already applying this pattern in both new and changed constructors - it would be good to be consistent and apply it everywhere.

Could we create/have a coding standards rule that could be optionally added to encourage using the new constructors?

@catch (#13)

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.

To me this seems to me like an unnecessary scope creep and distraction.
The issue has already filled up with comments that are not related to constructor property promotion.

Imo it is good practice to do incremental changes in separate issues, which reduce the scope of discussion and decisions in each issue.

Regarding the existing MR:
I prefer very much to have these constructor signatures broken to multiple lines:

  public function __construct(
    protected RootPackageInterface $rootPackage,
    protected string $eventName,
  ) {

This keeps it more readable, and also makes future MRs easier to review that want to add `readonly`, or `#[Autowire(..)]` attribute, or that want to add or remove parameters.

Then for parameter attributes we can either keep them on the same line or on a separate line.

  public function __construct(
    protected RootPackageInterface $rootPackage,
    #[Autowire(..)]
    protected string $eventName,
  ) {

or

  public function __construct(
    protected RootPackageInterface $rootPackage,
    #[Autowire(..)] protected string $eventName,
  ) {

I also created a coding standards issue regarding the line breaks.
#3458314: Convention or recommendation for line breaks in constructor parameter signature with promoted properties →

I don't think we need to block this on adding more verbiage to the coding standard, we already did https://www.drupal.org/node/3396762 → which covers things enough for me - i.e. in this issue we can use multiple lines.

We might need a coding standards issues for multi-line method signatures with parameter attributes but we hardly have any of those so it shouldn't be blocking.