Skip to content

Fix '?' in ReflectionNamedType::getName() from ReflectionProperty::getSettableType() #19201

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 2 commits into from
Closed

Fix '?' in ReflectionNamedType::getName() from ReflectionProperty::getSettableType() #19201

wants to merge 2 commits into from
+59 −3

Conversation

iluuu1994
Copy link
Member

No description provided.

DanielEScherzer
DanielEScherzer reviewed Jul 21, 2025
View reviewed changes
ext/reflection/tests/gh19187.phpt
echo $reflProp->getSettableType()->getName(), "\n";

?>
--EXPECT--
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, can you also include the output of __toString() and allowsNull() to confirm that the nullability is still communicated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Also added a test for the fallback path when no set is present.

@DanielEScherzer
Copy link
Member

Should this target master rather than 8.4, given that it changes the output? Otherwise looks good to me

@iluuu1994
Copy link
Member Author

Should this target master rather than 8.4, given that it changes the output?

I don't know whether this qualifies as a bug. When I implemented this function I wasn't aware "legacy behavior" is still widely applied, and it does seem like getSettableType() is the oddball. If you prefer I can target master.

@DanielEScherzer
Copy link
Member

Should this target master rather than 8.4, given that it changes the output?

I don't know whether this qualifies as a bug. When I implemented this function I wasn't aware "legacy behavior" is still widely applied, and it does seem like getSettableType() is the oddball. If you prefer I can target master.

Given that this might be a silent failure for any code checking the result and expecting it not to change without notice, I'd say that this should target master

@iluuu1994
Copy link
Member Author

Okay, I'll merge this into master instead then.

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

@DanielEScherzer DanielEScherzer DanielEScherzer approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@iluuu1994 @DanielEScherzer