On Wed, Apr 29, 2020 at 11:07 AM Nikita Popov <nikita....@gmail.com> wrote:
> On Wed, Apr 29, 2020 at 10:56 AM Benjamin Eberlei <kont...@beberlei.de> > wrote: > >> >> >> On Wed, Apr 29, 2020 at 9:47 AM Nicolas Grekas < >> nicolas.grekas+...@gmail.com> wrote: >> >>> > I think it might be best to apply to "both" and provide an isPromoted() >>> > method on both ReflectionParameter and ReflectionProperty. Any code >>> that >>> > wishes to validate the allowed positions of an attribute can then skip >>> > properties/parameters that report isPromoted() as true, to avoid >>> reporting >>> > false positives. >>> > >>> >>> That sounds good. Deal on my side. >>> >> >> Just to mention, any approach here potentially conflicts with anything we >> consider for potential target validation on attributes, i.e. declaring for >> an attribute that it is only allowed on a property OR an argument. >> >> At the point constructor promotion happens, we can also not look into the >> attribute to see if its target=property or target=parameter, because this >> would require triggering autoloader. >> > > Does it really conflict though? Can't the target validation just ignore > invalid attributes when promotion is involved (or rather, only check that > the attribute is valid for either property or parameter, but not > necessarily both of them)? > Since target validation would happen on ReflectionAttribute::newInstance alongside other validation already proposed, this means there is already the ReflectionAttribute instantiated, so we could only avoid throwing an exception, but the code might still get an attribute returned that doesn't belong there. The technical problem here is that we defer the validation to newInstance(), and not already during getAttributes(), to avoid autoloading. It looks like we can either have target validation and have to move validation to Reflection*::getAttributes(), or we can't have the validation. The same problem would essentially appear with a "repeatable=true/false" feature that prevents the same attribute from being declared multiple times. Its validation should also better be done at Reflection*::getAttributes(). Maybe to allow for access to attributes without validation we should instead have `Reflection*::getAttributes(ReflectionAttribute::FLAGS_NO_VALIDATION)` and don't defer validation to newInstance() by default? > > Nikita >
