Skip to content
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

Export AST for default value strings in reflection #7540

Closed
wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Oct 1, 2021

When dumping default values in ReflectionXXX::__toString(), for
expression initializers print the AST export instead of trying to
evaluate the expression. With the introduction of "new in
initializers" the result of the evaluation will commonly not be
printable at all, and "__toString" will throw an exception, which
is not particularly useful. Using the AST export also provides more
information on how the parameter was originally declared, e.g. it
preserves the fact that a certain constant was used.

When dumping default values in ReflectionXXX::__toString(), for
expression initializers print the AST export instead of trying to
evaluate the expression. With the introduction of "new in
initializers" the result of the evaluation will commonly not be
printable at all, and "__toString" will throw an exception, which
is not particularly useful. Using the AST export also provides more
information on how the parameter was originally declared, e.g. it
preserves the fact that a certain constant was used.
@nikic
Copy link
Member Author

nikic commented Oct 1, 2021

@krakjoe @ramsey @patrickallaert This addresses an issue that Symfony encountered with PHP 8.1. While it was possible for Reflection __toString() to throw before (use of undefined constant in default value), with "new in initializers" in 8.1 this situation becomes much more common and makes __toString() output quite unreliable. Printing the original expression instead avoids any error conditions in this code. I also think it generally makes more sense for a debug dump.

Any objections to making this change?

@krakjoe
Copy link
Member

krakjoe commented Oct 1, 2021

No objections.

@krakjoe krakjoe added the Feature label Oct 1, 2021
@ramsey
Copy link
Member

ramsey commented Oct 1, 2021

No objections from me, either

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

Successfully merging this pull request may close these issues.

3 participants