Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120456
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>
In-Reply-To: <23DF3A9A-9F11-4786-A011-41AE0FA9CFA9@koalephant.com>
Date: Tue, 30 May 2023 16:49:38 +0200
Message-ID: <CAB-Bujb9Hp+UoPy2Uxft8SuYaYo5fX1N_SD5vLeiAWmFdUkLSA@mail.gmail.com>
To: Stephen Reay <php-lists@koalephant.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 15:14, Stephen Reay <php-lists@koalephant.com> wrote=
:
>
> (Resending to the list without all the history because qmail complained a=
bout message size)
>
>
> >>
> >> Hi Andreas,
> >>
> >> I too have wondered (and I think asked in room11?) about such a concep=
t. >From memory the general response was =E2=80=9Cjust do it in userland wi=
th a wrapper=E2=80=9D so its good to see someone else is interested in this=
 being part of the language.
> >>
> >> While I agree that it=E2=80=99s most useful if the `Reflector` instanc=
e is available in the constructor, I=E2=80=99m not keen on the proposed mag=
ic =E2=80=9Cskipping=E2=80=9D of arguments as you suggest. It seems way too=
 easy to confuse someone (particularly if the attribute class itself has re=
ason to be instantiated directly in code)
> >
> > Good point! Almost made me change my mind completely. But I already
> > changed it back :)
> >
> > When instantiating in code, the "real" signature would have to be
> > used, and the reflector argument passed explicitly.
>
>
> That=E2=80=99s kind of my point: it=E2=80=99s not super intuitive why (or=
 the specifics of how) it=E2=80=99s being skipped when it=E2=80=99s an attr=
ibute, vs when it=E2=80=99s instantiated from code. What if someone specifi=
es an argument with the same name? If they specify args without names, can =
they just use null for that? Etc.

I agree it could be confusing.
But for the named args, I think it is quite obvious:

#[Attribute]
class A {
  public readonly array $moreArgs;
  public function __construct(
    public readonly string $x,
    // Reflector parameter can be last required parameter, BUT
    #[AttributeDeclaration] public readonly \ReflectionClass $class,
    // Optional parameters have to be after the required reflector paramete=
r.
    public readonly ?string $y =3D NULL,
    // Variadic parameter must be last.
    string ...$moreArgs,
  ) {
    $this->moreArgs =3D $moreArgs;
  }
}

#[A('x', 'y', 'z')]  // -> new A('x', $reflector, 'y', 'z')
#[A(x: 'x', y: 'y')]  // -> new A('x', $reflector, 'y')
#[A(x: 'x', class: new ReflectionClass('C'))]  // -> new A('x', new
ReflectionClass('C'))

We _could_ say that explicitly passing a value for the reflector in an
attribute declaration is forbidden.
Or we allow it, then an explicit value would simply overwrite the
implicit value.

If we use placeholder syntax, the above examples would look like this:

#[A('x', ?, 'y', 'z')]  // -> new A('x', $reflector, 'y', 'z')
#[A(x: 'x', class: ?, y: 'y')]  // -> new A('x', $reflector, 'y')
#[A(x: 'x', class: new ReflectionClass('C'))]  // -> new A('x', new
ReflectionClass('C'))


>
> > This would be
> > useful for unit tests that want to replicate the realistic behavior.
> > Also it guarantees that the code of the attribute class can really
> > count on this value to not be null, no matter how the class is
> > instantiated.
> >
> >
>
> I would expect that whether the Reflector object is required is simply a =
matter of whether or not the parameter is nullable.
> If it=E2=80=99s not nullable, then yes, the explicit instantiation call w=
ill need to supply it at the correct location. If it=E2=80=99s only require=
d when created from attribute usage, then it would accept null, and the con=
structor would have appropriate logic to handle that.
>

Yes.
But I would expect the common practice to be to make it required,
because then the constructor code will be simpler.

>
>
> >>
> >> I think a better approach would be to suggest authors put the paramete=
r at the *end* of the parameter list, so that no =E2=80=98skipping' is requ=
ired when passing arguments without names (or put it where you like if you=
=E2=80=99re always using named arguments)
> >
> > If I understand correctly, the proposal would technically not change,
> > we just add a recommendation.
>
> Technically, yes =E2=80=9Cmy way=E2=80=9D would work fine with the propos=
al you=E2=80=99ve suggested, if I choose to always put the parameter marked=
 by #[ReflectionContext] last.

As above, the problem with this would be optional and variadic
parameters, which have to come after a required reflector parameter.

>
> I=E2=80=99m just concerned about confusing usage if =E2=80=9Cinsert this =
parameter anywhere=E2=80=9D is the =E2=80=98recommended=E2=80=99 (i.e. docu=
mented example) way to use this feature.
>
> Even with that concern, I still prefer this to most other solutions menti=
oned so far, for the same reasons: they=E2=80=99re all some degree of magic=
.
>
> The only other solution I can think of that=E2=80=99s less =E2=80=9Cmagic=
=E2=80=9D and more explicit, is (and I have no idea if this is even feasibl=
e technically) to introduce a builtin trait for attribute classes to use, p=
roviding a protected method or property that gives access to the Reflector =
(how the trait has access is not really important, I assume it can be assig=
ned to the object somehow before the constructor is called). I guess this c=
ould also be an abstract class, but a trait makes it much easier to adopt s=
o that would be my preferred approach.
>
> So something like
>
> trait AttributeReflector {
>     protected function getReflector(): \Reflector {
>         // do internal stuff
>     }
> }
>
> #[Attribute]
> class Foo {
>     Use \AttributeReflector;
>
>     public readonly string $name;
>
>     function __construct(?string $name =3D null) {
>         $this->name =3D $name ?? $this->getReflector()->name;
>     }
> }

I was also considering this, but there is a problem.
When you instantiate the attribute class outside an attribute
declaration, how do you tell it about a required reflector?
This would be relevant e.g. during unit tests.

The constructor parameter provides a clean way to pass a custom reflector.
But with the ->getReflector(), the reflector would already be
magically added to the object before the constructor is executed. This
would be impossible to replicate in custom code outside an attribute
declaration.

---

Cheers
Andreas


>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>