On Tue, Sep 29, 2020 at 3:57 PM Benjamin Eberlei <kont...@beberlei.de>
wrote:
> On Tue, Sep 29, 2020 at 3:45 PM Larry Garfield <la...@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.mo...@gmail.com
> > >
> > > wrote:
> > >
> > > > On Mon, 28 Sep 2020 at 15:17, Nicolas Grekas <
> > nicolas.grekas+...@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
