Hello all,
A while back, I wrote a lengthy post about suggested improvements for reflection, but I would like to come back to address additional methods for dealing with attributes. (I have an open git issue here: https://github.com/php/php-src/issues/8489)
I'd like to introduce the following three methods on all reflect classes that have attributes:
hasAttribute(string $name, int $flags = 0): bool
Uses the same filtering as getAttributes(), but returns true if a match is found, false otherwise.
getAttribute(string $name, int $flags = 0): ?ReflectionAttribute
Also uses the same filtering as getAttributes(), except that it will return an instance of ReflectionAttribute if one is found, null if none are found, and will throw an exception of more than 1 is found.
getNumberOfAttributes(?string $name = null, int $flags = 0): int
Again, uses the same filtering as getAttributes(), and is to attributes what getNumberOfParameters() is to parameters. I appreciate that this methods use may not be obvious, but I wanted to include it for consistency.
I'm relatively confident that I have worked out the best method of implementing these, and contributors have agreed to merge a PR, which I will start shortly, should there be no objections.
Best Regards,
Ollie Read
Hello all,
A while back, I wrote a lengthy post about suggested improvements for
reflection, but I would like to come back to address additional methods
for dealing with attributes. (I have an open git issue here:
https://github.com/php/php-src/issues/8489)I'd like to introduce the following three methods on all reflect
classes that have attributes:hasAttribute(string $name, int $flags = 0): bool
Uses the same filtering as getAttributes(), but returns true if a match
is found, false otherwise.getAttribute(string $name, int $flags = 0): ?ReflectionAttribute
Also uses the same filtering as getAttributes(), except that it will
return an instance of ReflectionAttribute if one is found, null if none
are found, and will throw an exception of more than 1 is found.getNumberOfAttributes(?string $name = null, int $flags = 0): int
Again, uses the same filtering as getAttributes(), and is to attributes
what getNumberOfParameters() is to parameters. I appreciate that this
methods use may not be obvious, but I wanted to include it for
consistency.I'm relatively confident that I have worked out the best method of
implementing these, and contributors have agreed to merge a PR, which I
will start shortly, should there be no objections.
Best Regards,
Ollie Read
I'm not opposed to these, but would this be a good time to also add an interface for attributable reflection objects, so that we can type against that? These methods would all then go on that interface.
--Larry Garfield
I'm not opposed to these, but would this be a good time to also add an interface for attributable reflection objects, so that we can type against that? These methods would all then go on that interface.
I was going for a tiered approach. Once these methods were in, I would propose adding an Attributable or ReflectorWithAttributes interface, as well as a separate proposal to have ReflectionAttribute aware of the Attributable/ReflectionWithAttributes that it came from.
What's the difference between this and what was proposed in
https://externals.io/message/120799 ?
My proposal adds 3 methods, not just one, and was a followup to something from a while ago.
I don't get why this wouldn't require an RFC.
It's adding missing methods that should realistically be there. It breaks nothing, nor does it introduce entirely brand new things. It's the same as getParameter()/hasParameter() https://github.com/php/php-src/pull/10431 and hasPrototype() https://github.com/php/php-src/pull/8487. I couldn't tell you exactly why, just that the core contributors said so.
FY I started working on the implementation., hopefully it will bring more arguments in favor of that RFC which I’m willing to submit asap as well.
Is it just the hasAttribute() method you're working on? I'm happy to help out if you need/want it. I was moments away from opening a PR.
Best Regards,
Ollie Read
What's the difference between this and what was proposed in
https://externals.io/message/120799 ? I don't get why this wouldn't require
an RFC.
What's the difference between this and what was proposed in
https://externals.io/message/120799 ? I don't get why this wouldn't require
an RFC.
FY I started working on the implementation., hopefully it will bring more arguments in favor of that RFC which I’m willing to submit asap as well.
— Robin Chalas
Robin Chalas
Principal Engineer @Les-Tilleuls.coop
03 66 72 43 94
robin@les-tilleuls.coop
82 Rue Winston Churchill - 59160 Lomme
Le 25 juil. 2023 à 22:24 +0200, David Gebler davidgebler@gmail.com, a écrit :
What's the difference between this and what was proposed in
https://externals.io/message/120799 ? I don't get why this wouldn't require
an RFC.
FY I started working on the implementation., hopefully it will bring more
arguments in favor of that RFC which I’m willing to submit asap as well.
For future reference, please just open a PR with the implementation before
writing an RFC for those kinds of things. As self-contained, small features
do not necessarily require them.
Best regards,
George P. Banyard
PS: Side note, please do not bottom post.
For future reference, please just open a PR with the implementation before writing an RFC for those kinds of things. As self-contained, small features do not necessarily require them.
Best regards,
George P. Banyard
PS: Side note, please do not bottom post
Alright, I will just ship the PR for hasAttribute() then.
Thanks for the hints and feedback.
And sorry for the bad posting style, I hope this one is ok.
-- Robin Chalas
Alright, I will just ship the PR for
hasAttribute()then.
Thanks for the hints and feedback.
And sorry for the bad posting style, I hope this one is ok.
In that case, are you happy for me to PR the other two methods I suggested, or would you like to handle that?
Best Regards,
Ollie Read
In that case, are you happy for me to PR the other two methods I suggested, or would you like to handle that?
Please go ahead! Thanks for caring.
And if you already have the patch ready for hasAttribute() too but just care enough to leave it to the wanna-be-first-time-contributor that I am, please consider sharing it to me so that I can compare with what I have once I’m back from vacation next week (and either come back to you with questions or incline myself :)). Or I’ll send you mine for early review when I’m back to keyboard if you’re ok.
Cheers
-- Robin Chalas
Le 3 août 2023 à 20:20 +0200, Ollie Read php@ollie.codes, a écrit :
Alright, I will just ship the PR for
hasAttribute()then.
Thanks for the hints and feedback.
And sorry for the bad posting style, I hope this one is ok.In that case, are you happy for me to PR the other two methods I suggested, or would you like to handle that?
Best Regards,
Ollie Read
Please go ahead! Thanks for caring.
And if you already have the patch ready for hasAttribute() too but just care enough to leave it to the wanna-be-first-time-contributor that I am, please consider sharing it to me so that I can compare with what I have once I’m back from vacation next week (and either come back to you with questions or incline myself :)). Or I’ll send you mine for early review when I’m back to keyboard if you’re ok.
I'll wait for your implementation, as all the methods will be virtually identical, so it's best to base it off your hasAttribute method.
Best Regards,
Ollie Read
What's the difference between this and what was proposed in
https://externals.io/message/120799 ? I don't get why this wouldn't
require
an RFC.
Because it frankly doesn't require an RFC.
I think people are getting confused as to when one is required.
Adding methods for completeness that should just be there and no-one
complains can just get added.
The email is a courtesy to know if some people are going to bikeshed.
Now, adding an interface is more of a change that probably warrants one,
but even then it is fuzzy.
Best,
George P. Banyard