Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112135
MIME-Version: 1.0
References: <CAOWwgpkDAq4d229Z3D3=xBWyX-z8N+qC-BG4j0uoZy4Xc6JLKQ@mail.gmail.com>
 <CAF+90c_8Q-tuE-waqLENoH=ZLknJZfZmiji1p0hqJWwAqSfieQ@mail.gmail.com>
 <CAOWwgpmMhKDoeawbN23nGuR7J6A=AUfbZ4W+PNUkFACMDmvkhg@mail.gmail.com>
 <CAEPJdunxkKSRTxUhT13F-NAO4+3wmMJ3tanfFYh-iV6JUpUQDA@mail.gmail.com>
 <CAOWwgpkrNAVMZ6yj5eZhkd=aCC=x71V8hsLKxvdhqHoR6DKUuQ@mail.gmail.com>
 <BYAPR05MB54782200D1C1505A4EF85BCFDE1E0@BYAPR05MB5478.namprd05.prod.outlook.com>
 <CALKiJKqCMPdEOA-2_LPyBYeRRDEGh9TGgpFp0hWhTCxhCrs1og@mail.gmail.com> <BYAPR05MB547858A38633C2AF75A8567CDE1A0@BYAPR05MB5478.namprd05.prod.outlook.com>
In-Reply-To: <BYAPR05MB547858A38633C2AF75A8567CDE1A0@BYAPR05MB5478.namprd05.prod.outlook.com>
Date: Wed, 28 Oct 2020 17:58:40 +0100
Message-ID: <CAOWwgpmREMkGHvAQMPuUsJckG-5AvGyvpxcc0nh-ZWq9PjuW8Q@mail.gmail.com>
To: Theodore Brown <theodorejb@outlook.com>
Cc: Rowan Tommins <rowan.collins@gmail.com>, PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000eb5ea705b2be1331"
Subject: Re: [PHP-DEV] List of attributes
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--000000000000eb5ea705b2be1331
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi all!

about why we'd need nested attributes, here is a discussion about nested
validation constraints:
https://github.com/symfony/symfony/issues/38503

You'll see that we're looking hard into ways around using nested
attributes, but they all fall short. We actually concluded that we should
not create a hacky syntax before php-internals settled on the topic.
I invite you to review the ideas and arguments posted there to see a real
world need on the topic (please don't mind sharing your thoughts about the
topic in the PR of course if you think you can help us move forward.)

See more comments below:

Le ven. 23 oct. 2020 =C3=A0 17:52, Theodore Brown <theodorejb@outlook.com> =
a
=C3=A9crit :

> On Tue, Oct 20, 2020 at 10:13 AM Rowan Tommins <rowan.collins@gmail.com>
> wrote:
>
> > On Mon, 19 Oct 2020 at 16:17, Theodore Brown <theodorejb@outlook.com>
> wrote:
> >
> > >
> > > In theory nested attributes could be supported in the same way with
> > > the `#[]` syntax, but it's more verbose and I think less intuitive
> > > (e.g. people may try to use the grouped syntax in this context, but
> > > it wouldn't work). Also the combination of brackets delineating both
> > > arrays and attributes reduces readability:
> > >
> > >     #[Assert\All([
> > >         #[Assert\Email],
> > >         #[Assert\NotBlank],
> > >         #[Assert\Length(max: 100)]
> > >     ])]
> > >
> >
> > I think you're presupposing how a feature would work that hasn't
> > even been specced out yet. On the face of it, it would seem logical
> > and achievable for the above to be equivalent to this:
> >
> >     #[Assert\All(
> >        #[
> >             Assert\Email,
> >             Assert\NotBlank,
> >             Assert\Length(max: 100)
> >        ]
> >     )]
> >
> > i.e. for a list of grouped attributes in nested context to be
> > equivalent to an array of nested attributes.
>
> Hi Rowan,
>
> The problem with this is that it results in inconsistent semantics,
> where the number of items in an attribute group results in a
> different type of value being passed. I.e. if you remove two of the
> three attributes from the list, suddenly the code will break since
> the `Assert\All` attribute is no longer being passed an array.
>

Yes.
This would be solved by NOT accepting #[] inside attribute declarations.
Since we borrowed from Rust, let's borrow this again:
http://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/htm=
l/reference/attributes.html#attributes

The Rust syntax accepts the #[] only on the outside of the declaration.
The example above should be written as:
     #[Assert\All([
             Assert\Email(),
             Assert\NotBlank(),
             Assert\Length(max: 100)
     ])]

The closing brackets () would be required to remove any ambiguity with
constants.
Since constant expressions won't ever allow functions in them, this isn't
ambiguous with function calls.



> // error when Assert\All is instantiated since it's no longer being
> // passed an array of attributes:
>
>     #[Assert\All(
>         #[
>             Assert\Email,
>         ]
>     )]
>
> So when removing nested attributes from a list such that there's only
> one left in the list, you'd have to remember to additionally wrap the
> attribute group in array brackets:
>
>     #[Assert\All(
>         [#[
>             Assert\Email,
>         ]]
>     )]
>
> And of course when you want to add additional attributes to a group
> that originally had only one attribute, you'd have to remember to
> remove the extra array brackets.
>
> Generally speaking, having the number of items in a list change the
> type of the list is a recipe for confusion and unexpected errors. So
> I think the only sane approach is to ban using the grouped syntax in
> combination with nested attributes.
>
> Unfortunately, no one thought through these issues when proposing to
> change the shorter attribute syntax away from @@ and add the grouped
> syntax, and now it seems like we're stuck with a syntax that is
> unnecessarily complex and confusing to use for nested attributes.
>
> > Unless nested attributes were limited to being direct arguments to
> > another attribute, the *semantics* of nested attributes inside
> > arrays would have to be defined anyway (e.g. how they would look in
> > reflection, whether they would be recursively instantiated by
> > newInstance(), etc).
>
> Yes, the exact semantics of nested attributes need to be defined, but
> this is a completely separate topic from the grouped syntax issue
> described above.
>
> As a user I would expect nested attributes to be reflected the same
> as any other attribute (i.e. as a `ReflectionAttribute` instance).
> Calling `getArguments` on a parent attribute would return an array
> with `ReflectionAttribute` objects for any nested attribute passed
> as a direct argument or array value.
>

I 100% agree with this.


On first thought I think it makes sense to recursively instantiate
> nested attributes when calling `newInstance` on a parent attribute.
> This would allow attribute class constructors to specify the type
> for each attribute argument. But again, this is a separate discussion
> from the issue of nested attribute syntax.
>

Also 100% agree with this.

Cheers,
Nicolas

--000000000000eb5ea705b2be1331--