-
Notifications
You must be signed in to change notification settings - Fork 8k
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.
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
Conversation
...tSettableType() Fixes phpGH-19187
eecf4f4 to
5f80c87
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Should this target master rather than 8.4, given that it changes the output? Otherwise looks good to me
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.
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 targetmaster.
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
Okay, I'll merge this into master instead then.
No description provided.