> > 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. >
My bad about constants, I was wrong, they already work as part of constant expressions as you highlighted. I was propagating this laziness requirement to nested attributes: they should also be represented as instances of ReflectionAttribute to me at some point. It would look important to me to be able to inspect a tree of attributes and decide what to do with each of them before actually instantiating them (and triggering the autoload cascade). I'm not convinced that nested attributes should be brought to us via an improvement of constant-expressions: first because that would break this laziness requirement, second because I just doubt that constant-expressions should support objects. About providing laziness for attributes, this could be done by ReflectionAttribute::getAttributes(): #[Foo(bar())] could lead to getAttribute returning an array containing a ReflectionAttribute instance for attribute "bar". Of course, this has many consequences and also many alternatives that'd need to be evaluated. I won't be able to lead an effort towards nested attributes in the short term, so I'll just let this on the table here :) Cheers, Nicolas > >> 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 >> >
