# Attributes and constructor property promotion

- Thread: https://externals.io/message/111942
- Original archive: https://news-web.php.net/php.internals/111942
- Started by Nikita Popov on 2020-09-28T10:36:33+00:00
- Messages: 20
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #111942 — Nikita Popov — 2020-09-28T10:36:33+00:00

URL: https://externals.io/message/111942#111942

Hi internals,

When the constructor property promotion landed, the question of how it
interacts with attributes on promoted properties did not get fully
resolved. See https://wiki.php.net/rfc/constructor_promotion#attributes for
what the issue is.

The behavior that landed was to apply the attribute is applied to both the
parameter and the property. However, this was with the understanding that
we may have to adjust the behavior later, in particular depending on how
the whole "attribute target validation" turns out. The way it is now, using
a property-only attribute on a promoted parameter may result in spurious
validation errors.

I wanted to bring up this topic now to make sure it's not forgotten... I
still don't really know what the best behavior here is.

Regards,
Nikita

---

## #111943 — Benjamin Eberlei — 2020-09-28T11:56:03+00:00

URL: https://externals.io/message/111942#111943 · In reply to: #111942

> Hi internals,
>
> When the constructor property promotion landed, the question of how it
> interacts with attributes on promoted properties did not get fully
> resolved. See https://wiki.php.net/rfc/constructor_promotion#attributes
> for what the issue is.
>
> The behavior that landed was to apply the attribute is applied to both the
> parameter and the property. However, this was with the understanding that
> we may have to adjust the behavior later, in particular depending on how
> the whole "attribute target validation" turns out. The way it is now, using
> a property-only attribute on a promoted parameter may result in spurious
> validation errors.
>
> I wanted to bring up this topic now to make sure it's not forgotten... I
> still don't really know what the best behavior here is.
>

imho, we should pick the 80% use-case and advise to desugar the code if
other behavior is desired. For me the 80% case is that the attribute only
applies to the property, not to the parameter.

>
> Regards,
> Nikita

---

## #111944 — Benjamin Morel — 2020-09-28T12:24:49+00:00

URL: https://externals.io/message/111942#111944 · In reply to: #111943

On Mon, 28 Sep 2020 at 13:56, Benjamin Eberlei <kontakt@beberlei.de>
wrote:


> imho, we should pick the 80% use-case and advise to desugar the code if
> other behavior is desired. For me the 80% case is that the attribute only
> applies to the property, not to the parameter.


+1 for the desugaring advice in this case, however as a matter of
consistency I'd rather have it either apply to both the property and the
parameter, or just throw an exception if used on promoted properties,
whatever the 80% use-case is (source?)

Kind regards,
— Benjamin

---

## #111946 — Benjamin Eberlei — 2020-09-28T13:08:30+00:00

URL: https://externals.io/message/111942#111946 · In reply to: #111944

On Mon, Sep 28, 2020 at 2:25 PM Benjamin Morel <benjamin.morel@gmail.com>
wrote:

>   On Mon, 28 Sep 2020 at 13:56, Benjamin Eberlei <kontakt@beberlei.de>
> wrote:
>
>
> > […]
>
>
> +1 for the desugaring advice in this case, however as a matter of
> consistency I'd rather have it either apply to both the property and the
> parameter, or just throw an exception if used on promoted properties,
> whatever the 80% use-case is (source?)
>

We should assume that there are very few attributes that make sense to
apply to *both* a property and an argument. I can't find any good example
where this makes sense.

Library developers shouldn't be forced to allow target argument only that
it works in the ctor promotion case.

I assume the 80% case is properties, because attributes did not have
docblock annotations yet, that means this use-case isn't even possible at
the moment. Yet annotations on properties are widespread (Doctrine ORM,
symfony validator, ...).


> Kind regards,
> — Benjamin

---

## #111947 — Nicolas Grekas — 2020-09-28T13:16:45+00:00

URL: https://externals.io/message/111942#111947 · In reply to: #111946

>
> > […]
> only
> > […]
>
> We should assume that there are very few attributes that make sense to
> apply to *both* a property and an argument. I can't find any good example
> where this makes sense.
>
> Library developers shouldn't be forced to allow target argument only that
> it works in the ctor promotion case.
>
> I assume the 80% case is properties, because attributes did not have
> docblock annotations yet, that means this use-case isn't even possible at
> the moment. Yet annotations on properties are widespread (Doctrine ORM,
> symfony validator, ...).
>

I'm 100% with Benjamin here, this is what will be the most useful to me
also.

---

## #111948 — Benjamin Morel — 2020-09-28T14:34:55+00:00

URL: https://externals.io/message/111942#111948 · In reply to: #111947

On Mon, 28 Sep 2020 at 15:17, Nicolas Grekas <nicolas.grekas+php@gmail.com>
wrote:

> I assume the 80% case is properties, because attributes did not have
> > […]
>
> I'm 100% with Benjamin here, this is what will be the most useful to me
> also.
>

To be clear, I don't have a strong opinion against yours, I'm just pointing
out the fact that even though it might be useful, it might also be
confusing and create yet another WTF moment in PHP for developers. Sure, it
might make more sense to apply to the property. Sure, so far annotations
weren't possible on parameters. But is that obvious to the average
developer writing the attribute? A few years down the road, DI containers
may have broad support for annotating parameters for injection. Will it
still be obvious then that an attribute on a promoted property applies to
the property only?

I do agree that applying the attribute to both the property and the
parameter will probably never be useful, though. So, throwing an exception
and forcing the de-sugaring feels like the most sensible thing to do for me
in this case!

— Benjamin

---

## #111949 — Benjamin Eberlei — 2020-09-28T17:06:24+00:00

URL: https://externals.io/message/111942#111949 · In reply to: #111948

On Mon, Sep 28, 2020 at 4:35 PM Benjamin Morel <benjamin.morel@gmail.com>
wrote:

> On Mon, 28 Sep 2020 at 15:17, Nicolas Grekas <nicolas.grekas+php@gmail.com>
> wrote:
>
> > […]
>
> To be clear, I don't have a strong opinion against yours, I'm just
> pointing out the fact that even though it might be useful, it might also be
> confusing and create yet another WTF moment in PHP for developers. Sure, it
> might make more sense to apply to the property. Sure, so far annotations
> weren't possible on parameters. But is that obvious to the average
> developer writing the attribute? A few years down the road, DI containers
> may have broad support for annotating parameters for injection. Will it
> still be obvious then that an attribute on a promoted property applies to
> the property only?
>
> I do agree that applying the attribute to both the property and the
> parameter will probably never be useful, though. So, throwing an exception
> and forcing the de-sugaring feels like the most sensible thing to do for me
> in this case!
>

I haven't checked if this is possible in the code doing the "desugering",
but what if we had an attribute on the constructor that could specify where
the attributes should apply to?

#[AttributePromotion(Attribute::TARGET_PROPERTY)]
public function __construct(#[Foo] public string $bar) {}

Then we could apply it to both by default, which is what is probably the
expected approach, and users could change it to apply only to properties,
which is what is the use-case that makes most sense.

>
> — Benjamin

---

## #111958 — Larry Garfield — 2020-09-29T13:45:12+00:00

URL: https://externals.io/message/111942#111958 · In reply to: #111949

> On Mon, Sep 28, 2020 at 4:35 PM Benjamin Morel <benjamin.morel@gmail.com>
> wrote:
> 
> > […]
> 
> I haven't checked if this is possible in the code doing the "desugering",
> but what if we had an attribute on the constructor that could specify where
> the attributes should apply to?
> 
> #[AttributePromotion(Attribute::TARGET_PROPERTY)]
> public function __construct(#[Foo] public string $bar) {}
> 
> Then we could apply it to both by default, which is what is probably the
> expected approach, and users could change it to apply only to properties,
> which is what is the use-case that makes most sense.
> 
> > […]

From a user experience POV, I'd almost expect the opposite.

If I mark the attribute as being for properties, it gets applied to the property.

If I mark the attribute as being for parameters, it gets applied to the parameter.

If I mark it for both, or don't restrict it at all, it applies to both.

That may not be how the logic is currently implemented but that's what as a user I'd find least-surprising.

--Larry Garfield

---

## #111959 — Benjamin Eberlei — 2020-09-29T13:57:14+00:00

URL: https://externals.io/message/111942#111959 · In reply to: #111958

On Tue, Sep 29, 2020 at 3:45 PM Larry Garfield <larry@garfieldtech.com>
wrote:

> 
> > […]
> nicolas.grekas+php@gmail.com>
> > […]
> possible at
> > […]
> ORM,
> > […]
> me
> > […]
> also be
> > […]
> Sure, it
> > […]
> annotations
> > […]
> containers
> > […]
> to
> > […]
> exception
> > […]
> for me
> > […]
> where
> > […]
>
> From a user experience POV, I'd almost expect the opposite.
>
> If I mark the attribute as being for properties, it gets applied to the
> property.
>
> If I mark the attribute as being for parameters, it gets applied to the
> parameter.
>
> If I mark it for both, or don't restrict it at all, it applies to both.
>
> That may not be how the logic is currently implemented but that's what as
> a user I'd find least-surprising.
>

The problem with this approach is that it would require autoloading the
attributes when they are assigned to either the internal property or
parameter struct, but we have the design goal *not* to trigger autoloading
unless newInstance() or getArguments() is called. What you could do in
userland code is handle this case yourself and never newInstance()
attributes that don't apply to the right "thing" (parameter vs property).
but that would defer the problem to userland with some annoying piece of
code.


> --Larry Garfield
>
> --
> 
> To unsubscribe, visit: https://www.php.net/unsub.php

---

## #111961 — Nikita Popov — 2020-09-30T10:45:09+00:00

URL: https://externals.io/message/111942#111961 · In reply to: #111959

On Tue, Sep 29, 2020 at 3:57 PM Benjamin Eberlei <kontakt@beberlei.de>
wrote:

> On Tue, Sep 29, 2020 at 3:45 PM Larry Garfield <larry@garfieldtech.com>
> wrote:
>
> > […]
> benjamin.morel@gmail.com
> > […]
> it
> > […]
> applies
> > […]
> "desugering",
> > […]
> the
> > […]
> properties,
> > […]
>
> The problem with this approach is that it would require autoloading the
> attributes when they are assigned to either the internal property or
> parameter struct, but we have the design goal *not* to trigger autoloading
> unless newInstance() or getArguments() is called. What you could do in
> userland code is handle this case yourself and never newInstance()
> attributes that don't apply to the right "thing" (parameter vs property).
> but that would defer the problem to userland with some annoying piece of
> code.
>

So, as there seems to be resistance to applying the attribute to properties
only I only see a couple of options:

1. Forbid combining attributes and promotion.

2. Relax attribute validation for this case, as implemented in
https://github.com/php/php-src/pull/6244. I think if we otherwise stick to
the current behavior, this is what we should do to avoid false-positive
errors.

3. Disambiguate what is desired somehow. __construct(#[PropAttr] public int
$x) vs __construct(public #[ParamAttr] int $x). This is ... a bit too much
magic :)

Nikita

---

## #111962 — Dik Takken — 2020-09-30T11:48:35+00:00

URL: https://externals.io/message/111942#111962 · In reply to: #111961

> So, as there seems to be resistance to applying the attribute to properties
> only I only see a couple of options:
> 
> 1. Forbid combining attributes and promotion.

That means we can still decide to allow it later, right? Which gives us
more time to think this through in stead of deciding in a hurry.

Regards,
Dik Takken

---

## #111963 — Nicolas Grekas — 2020-09-30T12:13:14+00:00

URL: https://externals.io/message/111942#111963 · In reply to: #111961

Le mer. 30 sept. 2020 à 12:45, Nikita Popov <nikita.ppv@gmail.com> a écrit :

> On Tue, Sep 29, 2020 at 3:57 PM Benjamin Eberlei <kontakt@beberlei.de>
> wrote:
>
> > […]
> have
> > […]
> (Doctrine
> > […]
> to
> > […]
> Will
> > […]
> do
> > […]
> as
> > […]
> autoloading
> > […]
>
> So, as there seems to be resistance to applying the attribute to properties
> only I only see a couple of options:
>
> 1. Forbid combining attributes and promotion.
>

That'd be quite deceptive to me.



> 2. Relax attribute validation for this case, as implemented in
> https://github.com/php/php-src/pull/6244. I think if we otherwise stick to
> the current behavior, this is what we should do to avoid false-positive
> errors.
>

This makes a lot of sense actually!
Just to be sure, "ignore the error" means that attributes that target
properties would be applied only to the property, and attributes that
target params only to the param, isn't it? Or does it mean that they would
still be applied to both, but not error would be raised?



> 3. Disambiguate what is desired somehow. __construct(#[PropAttr] public int
> $x) vs __construct(public #[ParamAttr] int $x). This is ... a bit too much
> magic :)
>

a bit too much magic I agree :)

Nicolas

---

## #111964 — Nikita Popov — 2020-09-30T12:44:18+00:00

URL: https://externals.io/message/111942#111964 · In reply to: #111963

On Wed, Sep 30, 2020 at 2:13 PM Nicolas Grekas <nicolas.grekas+php@gmail.com>
wrote:

>
>
> Le mer. 30 sept. 2020 à 12:45, Nikita Popov <nikita.ppv@gmail.com> a
> écrit :
>
> > […]
>
> That'd be quite deceptive to me.
>
>
>
> > […]
>
> This makes a lot of sense actually!
> Just to be sure, "ignore the error" means that attributes that target
> properties would be applied only to the property, and attributes that
> target params only to the param, isn't it? Or does it mean that they would
> still be applied to both, but not error would be raised?
>

Unfortunately they would still be applied to both, but without error. As
Benjamin mentioned before, there's no way to prevent applying them to both
without loading the attribute definition and checking the targets on it.

Nikita

---

## #112015 — Nikita Popov — 2020-10-06T14:54:15+00:00

URL: https://externals.io/message/111942#112015 · In reply to: #111942

> Hi internals,
>
> When the constructor property promotion landed, the question of how it
> interacts with attributes on promoted properties did not get fully
> resolved. See https://wiki.php.net/rfc/constructor_promotion#attributes
> for what the issue is.
>
> The behavior that landed was to apply the attribute is applied to both the
> parameter and the property. However, this was with the understanding that
> we may have to adjust the behavior later, in particular depending on how
> the whole "attribute target validation" turns out. The way it is now, using
> a property-only attribute on a promoted parameter may result in spurious
> validation errors.
>
> I wanted to bring up this topic now to make sure it's not forgotten... I
> still don't really know what the best behavior here is.
>
> Regards,
> Nikita
>

I want to bump this topic, we should resolve this one way or another soon.
My two proposals are:

https://github.com/php/php-src/pull/6244: Suppress target validation errors
for promoted properties (for property/parameter targets, the rest gets
validated as usual).

https://github.com/php/php-src/pull/6285: Forbid use of attributes on
promoted properties altogether, punting on making a decision here.

Regards,
Nikita

---

## #112016 — Sara Golemon — 2020-10-06T15:15:15+00:00

URL: https://externals.io/message/111942#112016 · In reply to: #111942

> When the constructor property promotion landed, the question of how it
> interacts with attributes on promoted properties did not get fully
> resolved. See https://wiki.php.net/rfc/constructor_promotion#attributes
> for
> what the issue is.
>
> The behavior that landed was to apply the attribute is applied to both the
> parameter and the property. However, this was with the understanding that
> we may have to adjust the behavior later, in particular depending on how
> the whole "attribute target validation" turns out. The way it is now, using
> a property-only attribute on a promoted parameter may result in spurious
> validation errors.
>
> I wanted to bring up this topic now to make sure it's not forgotten... I
> still don't really know what the best behavior here is.
>
>
My opinion on constructor property promotion (CPP) is that it's something
for small value object classes and should probably be regarded as
code-smell on larger classes. At the same time, annotations belong with
more complex objects and not so much with small "struct-like" classes.

Given that position, I think we should err towards strictness in how
attributes are applied to CPP declarations.  That is, we should require
them to be meaningfully applicable to both arguments and properties in
order to be used in a CPP context.  If that's a problem for the consumer,
then they should avoid use of CPP.

-Sara

---

## #112017 — Andreas Leathley — 2020-10-06T16:20:31+00:00

URL: https://externals.io/message/111942#112017 · In reply to: #112016

> My opinion on constructor property promotion (CPP) is that it's something
> for small value object classes and should probably be regarded as
> code-smell on larger classes. At the same time, annotations belong with
> more complex objects and not so much with small "struct-like" classes.
>
> Given that position, I think we should err towards strictness in how
> attributes are applied to CPP declarations.  That is, we should require
> them to be meaningfully applicable to both arguments and properties in
> order to be used in a CPP context.  If that's a problem for the consumer,
> then they should avoid use of CPP.
>
> -Sara

The current usage of annotations is quite often with small struct-like
classes though - I mainly use annotations (and will use attributes) for
data that needs to be validated, or entity-like classes that contain
data. Those classes are small and simple and would benefit greatly from
CPP and attributes used in combination, so it would be a pity to make
that impossible.

 From my understanding suppressing the validation errors in this
particular case would be a good solution, or are there any serious
downsides to that?

---

## #112018 — Rowan Tommins — 2020-10-06T16:58:57+00:00

URL: https://externals.io/message/111942#112018 · In reply to: #112017

> From my understanding suppressing the validation errors in this
> particular case would be a good solution, or are there any serious
> downsides to that?
>


The downside presumably is that a library author could implement an
attribute with a property-only restriction, and therefore write code
handling parameter attributes with the assumption that that attribute won't
be present. If a consumer of the library makes use of constructor property
promotion, that assumption can be violated. I can't quite picture what that
code would look like, though, since it's always necessary to filter the
list of attributes to exclude those from other libraries.

Regards,
-- 
Rowan Tommins
[IMSoP]

---

## #112019 — Benjamin Eberlei — 2020-10-06T21:24:16+00:00

URL: https://externals.io/message/111942#112019 · In reply to: #112017

> 
> > […]
>
> The current usage of annotations is quite often with small struct-like
> classes though - I mainly use annotations (and will use attributes) for
> data that needs to be validated, or entity-like classes that contain
> data. Those classes are small and simple and would benefit greatly from
> CPP and attributes used in combination, so it would be a pity to make
> that impossible.
>
>  From my understanding suppressing the validation errors in this
> particular case would be a good solution, or are there any serious
> downsides to that?
>

This is my argument as well. Attributes + CPP are a powerful combination.

Attributes are an important use-csae for data transfer objects, to
configure validation or serialization for example:

class ChangeFirstNameRequest
{
    public function __construct(
        #[Validate\IsPositive]
        public int $id;
        #[Validate\NotBlank]
        #[Json("first_name")]
         public string $firstName;
    ) {}
}


> --
> 
> To unsubscribe, visit: https://www.php.net/unsub.php

---

## #112020 — Markus Fischer — 2020-10-07T07:22:06+00:00

URL: https://externals.io/message/111942#112020 · In reply to: #112016

Hi!


> My opinion on constructor property promotion (CPP) is that it's something
> for small value object classes and should probably be regarded as
> code-smell on larger classes. At the same time, annotations belong with
> more complex objects and not so much with small "struct-like" classes.

Just wanted to chime in on this:

I can foresee already the impact CPP will have on dependency injection: 
sometimes you just have complex business logic with lots of (auto-wired) 
dependencies and have to pull in n+1 classes into the constructor.

Currently 5+ deps create 3*(5+) codelines for not much value. My point 
is that I see much more value _outside_ of "value object classes" with 
auto-wiring, reducing boilerplate _a lot_.


That said: I err on the side of forbidding it, if we've no clear 
consensus how to do it!

thanks,
- Makrs

---

## #112021 — Nikita Popov — 2020-10-07T08:01:44+00:00

URL: https://externals.io/message/111942#112021 · In reply to: #112018

On Tue, Oct 6, 2020 at 6:59 PM Rowan Tommins <rowan.collins@gmail.com>
wrote:

> 
>
> > […]
>
>
> The downside presumably is that a library author could implement an
> attribute with a property-only restriction, and therefore write code
> handling parameter attributes with the assumption that that attribute won't
> be present. If a consumer of the library makes use of constructor property
> promotion, that assumption can be violated. I can't quite picture what that
> code would look like, though, since it's always necessary to filter the
> list of attributes to exclude those from other libraries.
>

Yeah, this is my own view as well. If an attribute appears in an unexpected
place, you'd just ignore it the same way you ignore all attributes you do
not "own". For manual implementations, you wouldn't even notice (because
for a property-only attribute, you'd only be looking at properties, duh)
and general attribute handling libraries can easily accommodate this case
(if it doesn't just happen automatically).

Regards,
Nikita