Skip to content

Fix GH-19747: Make ReflectionConstant non-final #19901

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix GH-19747: Make ReflectionConstant non-final #19901

wants to merge 1 commit into from

Conversation

sasezaki
Copy link
Contributor

fix #19747

@sasezaki
Copy link
Contributor Author

I created this PR based on the following issue, so I would to mention @Girgias

Roave/BetterReflection#1518

@Girgias
Copy link
Member

Girgias commented Sep 21, 2025

For my own use case I don't need this class to be non-final as I don't use the BetterReflection Adapter classes.

But I don't understand why the constant one needs to be a non-final but ReflectionFiber can still be final?

TimWolla
TimWolla previously requested changes Sep 21, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

#19747 (comment)

@sasezaki
Copy link
Contributor Author

I would like to close and withdraw this for now.
In the previous #7520, there was a use case where it was needed for PHPStan, but for this ReflectionConstant, there doesn't seem to be any case where it's actually needed at the moment.

@sasezaki sasezaki closed this Sep 21, 2025
@iluuu1994 iluuu1994 mentioned this pull request Sep 21, 2025
Closed
@iluuu1994 iluuu1994 reopened this Sep 22, 2025
@iluuu1994 iluuu1994 changed the base branch from PHP-8.4 to master September 22, 2025 15:29
@iluuu1994 iluuu1994 force-pushed the non-final-reflection-constant branch from 109b5b4 to 7445ce5 Compare September 22, 2025 15:31
@iluuu1994 iluuu1994 requested a review from a team September 22, 2025 15:33
edorian
edorian approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@edorian edorian left a comment

Choose a reason for hiding this comment

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

No RM objections

@iluuu1994 iluuu1994 closed this in 78c997f Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edorian edorian edorian approved these changes

@TimWolla TimWolla TimWolla left review comments

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

@DanielEScherzer DanielEScherzer Awaiting requested review from DanielEScherzer DanielEScherzer is a code owner

Assignees
No one assigned
Labels
Extension: reflection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make ReflectionConstant non-final
5 participants
@sasezaki @Girgias @TimWolla @edorian @iluuu1994