Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:111961
MIME-Version: 1.0
References: <CAF+90c8w3Aw9gbdFYV+Qm7Gyo0187GUMS2PmuUzVkW78JQ_Q_w@mail.gmail.com>
 <CAEPJdunXkRw7UVrg=sm=dSSJ_Fd5VnBsd0oW9MGfK4NNGDyBTQ@mail.gmail.com>
 <CAG9XoMTtLcQ6jS_fCqNsL41q48zDvvfMZZjbwvRvOT5MJmCzAQ@mail.gmail.com>
 <CAEPJdunAjvfZgV52XHoaQJDzXz12K4m0suoqGdFitCT7QR7XAA@mail.gmail.com>
 <CAOWwgpn457VLk1gOgqXUXZ8OP=pMqF7YW4FG-oM5yZ=RvFpu5A@mail.gmail.com>
 <CAG9XoMT+psKRLSUo070dNouQkeWBEXSLU=5c38L6Cnii4rNsnQ@mail.gmail.com>
 <CAEPJdu=dZPQbeCktySON=yVCpMgURqu7xkEM_PEZVDt=oqVVZQ@mail.gmail.com>
 <fe377540-b237-404c-8bfc-aaca9de57baa@www.fastmail.com> <CAEPJdukUzFtHvLW3f6nm8zhFQxTpZXymvjY+gVnd9h-w-ev=8w@mail.gmail.com>
In-Reply-To: <CAEPJdukUzFtHvLW3f6nm8zhFQxTpZXymvjY+gVnd9h-w-ev=8w@mail.gmail.com>
Date: Wed, 30 Sep 2020 12:45:09 +0200
Message-ID: <CAF+90c_cjTnP_e_x4ucmmnSNGR168D7vZxKxgF6cR6f84ut2Wg@mail.gmail.com>
To: Benjamin Eberlei <kontakt@beberlei.de>
Cc: Larry Garfield <larry@garfieldtech.com>, php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000d9d62405b0859897"
Subject: Re: [PHP-DEV] Re: Attributes and constructor property promotion
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000d9d62405b0859897
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 ha=
ve
> > > >>> docblock annotations yet, that means this use-case isn't even
> > possible at
> > > >>> the moment. Yet annotations on properties are widespread (Doctrin=
e
> > 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. Wil=
l
> 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 d=
o
> > 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.
> > >
> > > >
> > > > =E2=80=94 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 autoloadin=
g
> 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

--000000000000d9d62405b0859897--