-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fixing incorrect error message when passing null to join/implode's array parameter #12683
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
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.
I think you misread the error message.
Related to #11991 |
Doing a simple if/else would be better IMHO. You coul keep the However, the issue @iluuu1994 is talking about (without me looking at test failures) is that the errors might reference the wrong variable+type name when providing an invalid type for the 1 parameter version. |
Got it. Just pushed the change.
I'll take a look at the failing tests and see if that's the case. |
Yes. That would cause
So I'll go back to my original approach, ok? |
2ecfccc
to
54ad47f
Compare
b7b2467
to
e983de1
Compare
If after this change everything is ok, I have one question: I'd like to merge the |
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.
@iluuu1994 do you think this is fine or should we still split ZPP into two arity cases?
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.
do you think this is fine or should we still split ZPP into two arity cases?
It's fine. I think that only the message may be adjusted.
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.
LGTM otherwise
a812eee
to
95b8db9
Compare
Are there any actions I need to take here or is this PR good? 😁 |
When passing
null
to the second parameter ofjoin
/implode
function, the error message said "string given" instead of "null given". This PR changes the error message to display the correct type.I also added the test to it in a separate file since the
implode1.phpt
was huge already and it doesn't follow the recommendation of having_error
suffix.Fixes #12682