On Mon, Sep 28, 2020 at 2:58 PM Nicolas Grekas <nicolas.gre...@gmail.com> wrote:
> > Hi Benjamin, hi everyone >>> >>> I'm wondering if the syntax that allows for several attributes is really >>> future-proof when considering nested attributes: >>> >>> >>> *1.* >>> #[foo] >>> #[bar] >>> >>> VS >>> >>> >>> *2.* >>> #[foo, bar] >>> >>> Add nested attributes to the mix, here are two possible ways: >>> >>> >>> *A.* >>> #[foo( >>> #[bar] >>> )] >>> >>> or >>> >>> >>> *B.* >>> #[foo( >>> bar >>> )] >>> >>> The A. syntax is consistent with the 1. list. >>> I feel like syntax B is not desired and could be confusing from a grammar >>> pov. >>> BUT in syntax 2., we allow an attribute to be unprefixed (bar), so that >>> syntax B is consistent with 2. >>> >>> Shouldn't we remove syntax 2. in 8.0 and consider it again when nested >>> attributes are introduced? >>> >>> I voted yes for syntax 2. when the attributes were using << >>. I would >>> vote NO now with the new syntax. >>> >>> Nicolas >>> >> >> As far as my understanding goes, if we introduce "nested" attributes, it >> will be in the form of relaxing constraints on constant expressions, i.e. >> by allowing you to write #[Attr(new NestedAttr)]. >> > > That's what I've read in previous discussions and in other replies in this > thread. > This would break the possibility to read annotations without failing hard > when a class is missing. > > I would much prefer a solution that preserves this capability (which is > the reason why we have ReflectionAttribute::newInstance(): unless this one > is called, no autoloading happens. > > To me, this means that "new Foo()" nested in an attribute can't be a > solution. > Neither should we be able to nest PHP constants there btw. > Can you clarify your reasoning here to get to this conclusion? because with #[Foo(Foo::BAR)] we also not trigger autoloading right now until newInstance() or getArguments() is called. This is the way it is working right now: https://gist.github.com/beberlei/8150f60abaab3b0a70ebb76ce6a379b0 Attributes allow constant expressions as arguments, the same which you can do in constant or property declarations for the default value. An approach with new NestedAttr() would work the same, only triggering autoload when argument needs to be evaluated for use. > > Since we borrowed the syntax to Rust, here is the syntax they use: > > > #[validate(length(min = 1), custom = "validate_unique_username")] > > I think this would fit quite nicely for us too, by turning eg length into > an instance of ReflectionAttribute. > > Note that a use case for nested attributes is discussed in > https://github.com/symfony/symfony/pull/38309, to replace things like: > /** > @Assert\All([ > @Assert\Email, > @Assert\NotBlank, > @Assert\Length(max=100) > ]) > */ > > We can work around of course, and we will for 8.0, but it would be nice to > have a plan forward because similar use cases (grouping attributes in a > wrapper attribute) are going to be pretty common IMHO. > > But we're getting a bit of topic. I'm fine keeping things as is for 8.0. I > just wanted to raise a point about the syntax for list of attributes but it > didn't get traction apparently. > > Cheers, > Nicolas >
