Skip to content

Bugfix 80097: Have ReflectionAttribute implement Reflector, __toString #6117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented Sep 12, 2020

https://bugs.php.net/bug.php?id=80097

Questions:

  • Is this the right representation? Arguments could potentially be very complex, I don't think they should be rendered?
  • what about attributes on all reflectors, should we print them as well? so when you echo a ReflectionFunction with an attribute for example.
  • What about php -r*?

@krakjoe krakjoe added this to the PHP 8.1 milestone May 21, 2021
@krakjoe
Copy link
Member

krakjoe commented May 21, 2021

@beberlei this looks like an easy win, can you move this forward please ?

(nobody complained, ship it, imo)

@nikic
Copy link
Member

nikic commented May 21, 2021

I think I complained at the time, but I changed my mind and this looks fine :)

@nikic
Copy link
Member

nikic commented May 21, 2021

https://bugs.php.net/bug.php?id=80097

Questions:

* Is this the right representation? Arguments could potentially be very complex, I don't think they should be rendered?

I think it would be good to include arguments. Maybe you can use the format_default_value logic to print a compact representation?

* what about attributes on all reflectors, should we print them as well? so when you echo a ReflectionFunction with an attribute for example.

I think that would make sense, yes.

* What about `php -r*`?

Don't think that's a concern, because attribute uses (which is what ReflectionAttribute is about) aren't distinct symbols. This is also kind of covered by the previous point.

@ramsey
Copy link
Member

ramsey commented Jun 9, 2021

@beberlei, this looks like it's waiting on some feedback from you before I can merge it in.

@beberlei
Copy link
Contributor Author

beberlei commented Jun 9, 2021

Ah will check in the next days

@beberlei beberlei marked this pull request as ready for review June 19, 2021 12:34
@beberlei
Copy link
Contributor Author

I'd like this to get merged to have Reflector::__toString support on ReflectionAttribute for now, and I will work on support for attributes on each other reflector in another PR in the next weeks when I have some time.

@beberlei beberlei force-pushed the ReflectionAttributeReflector branch from be19048 to 3d9c603 Compare June 19, 2021 19:28
Argument #1 [ b = 1234 ]
}
}
Attribute [ Baz ] {
Copy link
Member

@nikic nikic Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay, but I wonder if it would make sense to print it as

Attribute [ Baz('foo', 1234) ]

or even

#[Baz('foo', 1234)]

instead?

Maybe this doesn't compose well when included in other reflection output though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the idea was that this gets its own _attribute_string at some point and nests into the existing reflection structures that look this way.

@nikic nikic closed this in bc39abe Jul 7, 2021
@derrabus
Copy link
Contributor

derrabus commented Jul 7, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants