Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120476
MIME-Version: 1.0
References: <CAB-BujaDnogpq7=BwwVbbT6XmBAAiDqQ3mDqgNccdzG1nW=WuQ@mail.gmail.com>
 <64EAB991-7082-47B8-B546-73CD08243C6E@koalephant.com> <CAB-BujYA+CG1GkS176eqsVkMrgQVqX=P8xNPz9zNgijE1uMC6w@mail.gmail.com>
 <23DF3A9A-9F11-4786-A011-41AE0FA9CFA9@koalephant.com> <CAB-Bujb9Hp+UoPy2Uxft8SuYaYo5fX1N_SD5vLeiAWmFdUkLSA@mail.gmail.com>
 <75a8db86-c91d-4f5e-b19e-e968926b4e8a@app.fastmail.com>
In-Reply-To: <75a8db86-c91d-4f5e-b19e-e968926b4e8a@app.fastmail.com>
Date: Tue, 30 May 2023 21:34:52 +0200
Message-ID: <CAB-Bujbmi7ns1Dqx8FdRrihsFa+6qWL=a0rmbK_HAW925T0VyA@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Declaration-aware attributes
From: andreas@dqxtech.net (Andreas Hennings)

On Tue, 30 May 2023 at 19:12, Larry Garfield <larry@garfieldtech.com> wrote=
:
>
> I've run into this issue in my attribute library as well (https://github.=
com/Crell/AttributeUtils).  What I do there is allow attributes to opt-in t=
o a callback method via interface.  For example:
>
> #[\Attribute]
> class AttribWithName implements FromReflectionClass
> {
>     public readonly string $name;
>
>     public function __construct(?string $name =3D null)
>     {
>         if ($name) {
>             $this->name =3D $name;
>         }
>     }
>
>     public function fromReflection(\ReflectionClass $subject): void
>     {
>         $this->name ??=3D $subject->getShortName();
>     }
> }

So it is technically from outside it is a setter, whereas from inside
it is not really.
Technically, this means you need a version of the attribute object
that can exist without the values from the reflector.
The constructor needs to initialize some "stub" values, until the
setter is called.
Every other method also needs to support the case when the setter has
not been called yet, or possibly throw an exception.
Also, your property type has to allow null, so `?string`, not `string`.

I usually try to avoid this, this is why I proposed the constructor paramet=
er.
This way, the object is never in an incomplete state.

(Side note: Personally I use the naming convention from*() for static
factory methods. I might use set*() for this one, but then again, it
is only a setter from the outside.)

>
> (Side note: This is why static analysis tools that forbid writing to read=
only properties except from the constructor are wrong; it's also an example=
 of where asymmetric visibility would be superior to readonly.  But I digre=
ss.)

Technically there is no guarantee that the setters will be called
before any other method, and only once.
If these methods can write on readonly properties, then any method can.
Unless we somehow mark these methods as special.

On the other hand, a wither method with "clone with" should be allowed
to work on readonly properties.
You could rewrite your method like this, once we have clone with:
(or use the old-school syntax but without readonly)

    public function withReflector(\ReflectionClass $subject): static
    {
        return ($this->name !=3D=3D NULL)
            ? $this
            : clone $this with (name: $subject->getShortName();
    }

Then in the discovery code:

    $attribute =3D $reflectionClass->getAttributes()[0]->newInstance();
    $attribute =3D $attribute->withReflector($reflectionClass);

>
> My preference would be for something along those lines to be implemented =
in core.
>
> Importantly, we *MUST NOT* design it such that the reflection object gets=
 assigned to a property of the attribute.  Reflection objects are not seria=
lizable.  Attributes will frequently be cached.  That means it forces the a=
ttribute author to make the property nullable AND then unset it sometime be=
fore the attribute gets serialized, or it will break.  That's a no-go.

There could be ways around this problem, but I agree we should avoid
it on design level.

>
> That's why I think an opt-in interface is the way to go.  It also avoids =
any confusion around the constructor parameters, which is, based on this th=
read, a confusing area. :-)

What do you think about a placeholder syntax to avoid confusion with a
skipped parameter?
Like

#[A('x', ?', 'y')]

>
> My second preference would be the ReflectionAttribute::inProgress() call =
in the constructor, or something like that.  I like that less because it's =
a stateful call, but it would also reduce the issue with readonly propertie=
s (as in the example above) by making both alternatives available in the co=
nstructor, so maybe it's an acceptable tradeoff.

I would like to avoid anything that is stateful or that leaves an
incomplete stub object.
(But I think I made this clear enough, so..)

>
> I can see an argument either direction here.
>
> --Larry Garfield
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>