Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110395
MIME-Version: 1.0
References: <CAEPJdum6xCwABjpor1YYvuosSB1dEb4xnCZEMABKsEEvNXvcQQ@mail.gmail.com>
 <CAEPJdukfeRs2H8+qsXpHZgTeEaZnvDYXUtCGOhTu1ZZDBwehHA@mail.gmail.com>
 <CANeKSiQK6mGwCANDB=O8JsLieC8JM1uGeuRoguTQPDBgqO553g@mail.gmail.com>
 <CAEPJdumeH+DY=v8xQyV=wYYPbO8eb60CVrHv-00JSD+gK8i49A@mail.gmail.com> <CAFp73Xvt9U5viFKJG66oLJPpm-v311RNmwAJ2J_zq_Fx5R7uWw@mail.gmail.com>
In-Reply-To: <CAFp73Xvt9U5viFKJG66oLJPpm-v311RNmwAJ2J_zq_Fx5R7uWw@mail.gmail.com>
Date: Sat, 6 Jun 2020 00:01:46 +0200
Message-ID: <CAEPJdum07Cgt-zYD2ODe+eu79D-3H1pDSkV=fFM9qgunmSC2KQ@mail.gmail.com>
To: "guilhermeblanco@gmail.com" <guilhermeblanco@gmail.com>
Cc: Benas IML <benas.molis.iml@gmail.com>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000ed98cf05a75d684e"
Subject: Re: [PHP-DEV] Re: [RFC] Amendments to Attributes
From: kontakt@beberlei.de (Benjamin Eberlei)

--000000000000ed98cf05a75d684e
Content-Type: text/plain; charset="UTF-8"

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.

>
> Cheers,
>
> On Thu, Jun 4, 2020 at 1:49 PM Benjamin Eberlei <kontakt@beberlei.de>
> wrote:
> >
> > On Thu, Jun 4, 2020 at 12:54 PM Benas IML <benas.molis.iml@gmail.com>
> wrote:
> >
> > > Thank you for the update! Given that there is still an open issue, is
> the
> > > RFC proposing flags or a separate `<<Repeatable>>` attribute?
> > >
> >
> > Good point, we came to the conclusion to simplify. Should attributes be
> in
> > the global namespace, then we shouldn't arbitrarily add more, so it will
> be
> > a flag.
> > At that point, because you rarely declare new flags we decided to merge
> > target and flags and only have one flag. You could do the following:
> >
> > <<PhpAttribute(self::TARGET_METHOD | self::IS_REPEATABLE)>>
> >
> > >
> > > Best regards,
> > > Benas
> > >
> > > On Thu, Jun 4, 2020, 12:29 PM Benjamin Eberlei <kontakt@beberlei.de>
> > > wrote:
> > >
> > >> I have changed back the rename from namespacing to
> Attributes\Attribute to
> > >> using just Attribute after a few discussions off list. The reasoning
> is
> > >> that it becomes more clear that a majority of core contributors
> strongly
> > >> prefers using the global namespace as the PHP namespace and opening up
> > >> this
> > >> point again makes no sense. So the state of the RFC is again what it
> was
> > >> when I originally posted it with renaming PhpAttribute to Attribute.
> > >>
> > >> Unless there is some new significant feedback I am going to open up
> this
> > >> RFC for voting on Monday next week.
> > >>
> > >> On Wed, May 20, 2020 at 7:07 PM Benjamin Eberlei <kontakt@beberlei.de
> >
> > >> wrote:
> > >>
> > >> > Hi everyone,
> > >> >
> > >> > the Attributes RFC was rather large already, so a few things were
> left
> > >> > open or discussions during the vote have made us rethink a things.
> > >> >
> > >> > https://wiki.php.net/rfc/attribute_amendments
> > >> >
> > >> > These points are handled by the Amendments RFC to Attributes:
> > >> >
> > >> > 1. Proposing to add a grouped syntax <<Attr1, Attr2>
> > >> > 2. Rename PhpAttribute to Attribute in global namespace
> (independent of
> > >> > the namespace RFC)
> > >> > 3. Add validation of attribute class targets, which internal
> attributes
> > >> > can do, but userland can't
> > >> > 4. Specification if an attribute is repeatable or not on the same
> > >> > declaration and fail otherwise.
> > >> >
> > >> > Each of them is a rather small issue, so I hope its ok to aggregate
> all
> > >> > four of them in a single RFC. Please let me know if it's not.
> > >> >
> > >> > greetings
> > >> > Benjamin
> > >> >
> > >>
> > >
>
>
>
> --
> Guilherme Blanco
> SVP Technology at Statflo Inc.
> Mobile: +1 647 232 5599
>

--000000000000ed98cf05a75d684e--