Everything

php

Attributes and constructor property promotion

php.internals

Nikita Popov

5 years ago

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

Benjamin Eberlei

5 years ago

On Mon, Sep 28, 2020 at 12:36 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

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

Benjamin Morel

5 years ago

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

Benjamin Eberlei

5 years ago

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:
>
>
>> 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?)
>

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

Nicolas Grekas

5 years ago

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

Benjamin Morel

5 years ago

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

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

Benjamin Eberlei

5 years ago

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

Larry Garfield

5 years ago

On Mon, Sep 28, 2020, at 12:06 PM, Benjamin Eberlei wrote:

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

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

Benjamin Eberlei

5 years ago

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

> On Mon, Sep 28, 2020, at 12:06 PM, Benjamin Eberlei wrote:
> > 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:
> > >
> > >> 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.
> > >>
> > >
> > > 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
>
> 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.

Nikita Popov

5 years ago

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:
>
> > On Mon, Sep 28, 2020, at 12:06 PM, Benjamin Eberlei wrote:
> > > 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:
> > > >
> > > >> 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.
> > > >>
> > > >
> > > > 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
> >
> > 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.
>

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

Nikita Popov

5 years ago

On Mon, Sep 28, 2020 at 12:36 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

> 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

Sara Golemon

5 years ago

On Mon, Sep 28, 2020 at 5:36 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> 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

Andreas Leathley

5 years ago

On 06.10.20 17:15, Sara Golemon wrote:

> 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?

Rowan Collins

5 years ago

On Tue, 6 Oct 2020 at 17:20, Andreas Leathley <a.leathley@gmx.net> wrote:

> 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]

Nikita Popov

5 years ago

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

> On Tue, 6 Oct 2020 at 17:20, Andreas Leathley <a.leathley@gmx.net> wrote:
>
> > 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.
>

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

Benjamin Eberlei

5 years ago

On Tue, Oct 6, 2020 at 6:21 PM Andreas Leathley <a.leathley@gmx.net> wrote:

> On 06.10.20 17:15, Sara Golemon wrote:
> > 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?
>

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;
) {}
}

Markus Fischer

5 years ago

Hi!

On 06.10.20 17:15, Sara Golemon wrote:

> 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