On Tue, May 30, 2023, at 2:49 PM, Andreas Hennings wrote:
> On Tue, 30 May 2023 at 15:14, Stephen Reay <php-li...@koalephant.com> wrote:
>>
>> (Resending to the list without all the history because qmail complained
>> about message size)
>>
>>
>> >>
>> >> Hi Andreas,
>> >>
>> >> I too have wondered (and I think asked in room11?) about such a concept.
>> >> >From memory the general response was “just do it in userland with a
>> >> wrapper” so its good to see someone else is interested in this being part
>> >> of the language.
>> >>
>> >> While I agree that it’s most useful if the `Reflector` instance is
>> >> available in the constructor, I’m not keen on the proposed magic
>> >> “skipping” of arguments as you suggest. It seems way too easy to confuse
>> >> someone (particularly if the attribute class itself has reason 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’s kind of my point: it’s not super intuitive why (or the specifics of
>> how) it’s being skipped when it’s an attribute, vs when it’s instantiated
>> from code. What if someone specifies 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 parameter.
> public readonly ?string $y = NULL,
> // Variadic parameter must be last.
> string ...$moreArgs,
> ) {
> $this->moreArgs = $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’s not nullable, then yes, the explicit instantiation call will need to
>> supply it at the correct location. If it’s only required when created from
>> attribute usage, then it would accept null, and the constructor 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 parameter
>> >> at the *end* of the parameter list, so that no ‘skipping' is required
>> >> when passing arguments without names (or put it where you like if you’re
>> >> always using named arguments)
>> >
>> > If I understand correctly, the proposal would technically not change,
>> > we just add a recommendation.
>>
>> Technically, yes “my way” would work fine with the proposal you’ve
>> 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’m just concerned about confusing usage if “insert this parameter anywhere”
>> is the ‘recommended’ (i.e. documented example) way to use this feature.
>>
>> Even with that concern, I still prefer this to most other solutions
>> mentioned so far, for the same reasons: they’re all some degree of magic.
>>
>> The only other solution I can think of that’s less “magic” and more
>> explicit, is (and I have no idea if this is even feasible technically) to
>> introduce a builtin trait for attribute classes to use, providing 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 assigned to the
>> object somehow before the constructor is called). I guess this could also be
>> an abstract class, but a trait makes it much easier to adopt so 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 = null) {
>> $this->name = $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.
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 to a callback method via interface. For example:
#[\Attribute]
class AttribWithName implements FromReflectionClass
{
public readonly string $name;
public function __construct(?string $name = null)
{
if ($name) {
$this->name = $name;
}
}
public function fromReflection(\ReflectionClass $subject): void
{
$this->name ??= $subject->getShortName();
}
}
(Side note: This is why static analysis tools that forbid writing to readonly
properties except from the constructor are wrong; it's also an example of where
asymmetric visibility would be superior to readonly. But I digress.)
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
serializable. Attributes will frequently be cached. That means it forces the
attribute author to make the property nullable AND then unset it sometime
before the attribute gets serialized, or it will break. That's a no-go.
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 thread, a
confusing area. :-)
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 properties (as in the
example above) by making both alternatives available in the constructor, so
maybe it's an acceptable tradeoff.
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
