On Fri, Jun 5, 2020 at 5:58 AM guilhermeblanco@gmail.com <
guilhermeblanco@gmail.com> wrote:
> Hi Benjamin,
>
> Overall, all these amendments are good in my opinion, but I'd like to
> challenge a few things:
>
> 1- On item 3, the acceptable targets would be: class, function,
> method, property, class constant, parameter or all.
> If possible, I'd like to ask if it's possible to expand this list and
> also allow attribute and constructor.
>
I think this is too specific to be valuable in the language, and it could
always be validated at the attribute reading level in userland.
One thing i was thinking now, what if attributes could be "Reflection
declaration aware", example:
interface ReflectorAwareAttribute
{
public function setReflector(Reflector $reflector);
}
class MyAttribute implements ReflectorAwareAttribute {
public function setReflector(Reflector $reflector) {
}
}
ReflectionAttribute::newInstance() would check for this interface and call
setReflector if present.
>
> 2- Also on item 3, the validation of PhpAttribute targets, it feels
> more natural to have this as an array instead of a bitwise or
> operator.
> Have you evaluated the performance penalty to judge your decision of
> bitwise vs array?
>
I feel this is subjective, PHP APIs generally use bitmasks for this kind of
differentiation and not an array of strings. An array of strings would have
the problem of the case with one target only: ["class"], which soon would
raise requests for a string "class" only, where a union type is probably
more "complex" than a bitmask.
>
> 3- Repeatability should be on its own PhpAttribute. It would not block
> the expansion of the repeatability in future efforts.
> One possibility could be group repeated attributes as another
> PhpAttribute. Example: Multiple <<Schedule>> be folded into a
> <<Schedules>>.
> This code:
>
> <<Schedule("0 0 12 * * MON-FRI")>>
> <<Schedule("0 0 18 * * SUN,SAT")>>
>
> Would be equivalent to (sorry if syntax is not 100% correct):
>
> <<Schedules([
> <<Schedule("0 0 12 * * MON-FRI")>>,
> <<Schedule("0 0 18 * * SUN,SAT")>>
> ])>>
>
> Considering:
>
> <<PhpAttribute(PhpAttribute::TARGET_CLASS)>>
> <<PhpRepeatable(Schedules::class)>>
> class Schedule { public string $cron; /* ... */ }
>
> <<PhpAttribute(PhpAttribute::TARGET_CLASS)>>
> class Schedules { public array value = []; // Holds an array of Schedule }
>
Without knowing at all how and if nested attributes get supported in the
future I feel this introduces a lot of complexity that is unnecessary.
If nested attributes are added at some point the validation could be done
in the container attributes constructor. Taking your example:
<<PhpAttribute>>
class Schedules
{
public function __construct(array $schedules) {
foreach ($schedules as $schedule) {
$this->addSchedule($schedule);
}
}
private function addSchedule(Schedule $schedule) {}
}
Any kind of language support for typed arrays would obviously simplify this
even more.
> 4- Now you might have recall about my initial thoughts on this
> subject, but inheritance is something that would be very interesting
> to see as part of this amendment.
> If we introduce something like <<PhpAttributeInherited>> to the
> Attribute definition, we could then mark the Attribute to be inherited
> to subclasses of attributed class, while keeping the default to do not
> inherit anything (like we have today).
>
An equivalent of Java Annotations @Inherited is not easily possible, so we
omitted it for now.
The reason is that the compile step does not validate attributes (unless
they are internal engine attributes, that can only be provided by
extensions). So it doesn't know at that point if the declared attribute
exists and what inherit rules it might hvae defined. One solution could be
to add a new flag to ::getAttributes($name, $flags =
ReflectionAttribute::INCLUDE_INHERITED) that performs the gathering of
inherited attributes, however that will have to trigger autoloading. The
complexity and the rare use case for me speaks against adding it at the
moment.
