On Wed, Apr 29, 2020 at 11:27 AM Benjamin Eberlei <kont...@beberlei.de> wrote:
> > > 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? > Performing validation when the getAttributes() call is performed does sound reasonable to me. We can also add a class flag to perform this validation only once (if it is successful), so the cost doesn't have to be paid by every single getAttributes() consumer. For the purpose of this RFC, I've now updated it to say that attributes will be applied to both properties and parameters ( https://wiki.php.net/rfc/constructor_promotion#attributes), but with an explicit note that we should change the behavior prior to the PHP 8 release if it turns out to be problematic, e.g. with regards to an attribute validation feature. I think this is something of a detail, and we'll be mostly fine whichever way we chose, but it's hard to say right now which option is best, in particular with regard to future attributes improvements. Regards, Nikita
