Everything

php

New reflection methods for working with attributes

php.internals

Ollie Read

3 years ago

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*

Larry Garfield

3 years ago

On Tue, Jul 25, 2023, at 6:09 PM, Ollie Read wrote:

> 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

Ollie Read

3 years ago

> 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*

David Gebler

3 years ago

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.

Robin Chalas

3 years ago

> 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 :

Girgias

3 years ago

On Tue, 25 Jul 2023 at 21:55, Robin Chalas <robin.chalas@gmail.com> wrote:

> 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.

Robin Chalas

3 years ago

>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

Ollie Read

3 years ago

> 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*

Robin Chalas

3 years ago

> 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 :

Ollie Read

3 years ago

> 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*

Girgias

3 years ago

On Tue, 25 Jul 2023 at 21:24, David Gebler <davidgebler@gmail.com> wrote:

> 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