Skip to content

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

Merged
merged 14 commits into from
Dec 1, 2023

Conversation

CViniciusSDias
Copy link
Contributor

When passing null to the second parameter of join/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

Copy link
Member

@iluuu1994 iluuu1994 left a 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.

@Girgias
Copy link
Member

Girgias commented Nov 16, 2023

Related to #11991

@Girgias
Copy link
Member

Girgias commented Nov 16, 2023

Doing a simple if/else would be better IMHO. You coul keep the == 1 condition for the first case.

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.

@CViniciusSDias
Copy link
Contributor Author

Doing a simple if/else would be better IMHO. You coul keep the == 1 condition for the first case.

Got it. Just pushed the change.

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.

I'll take a look at the failing tests and see if that's the case.

@CViniciusSDias
Copy link
Contributor Author

CViniciusSDias commented Nov 16, 2023

Yes. That would cause implode('') to throw:

implode(): Argument #1 ($separator) must be of type array, string given

So I'll go back to my original approach, ok?

@CViniciusSDias
Copy link
Contributor Author

If after this change everything is ok, I have one question:

I'd like to merge the implode_ and join_ testcases so we don't have them with different function names. Would that be ok?
And if that's ok, should I do it in a separate PR?

Copy link
Member

@Girgias Girgias left a 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?

Copy link
Member

@iluuu1994 iluuu1994 left a 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.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@CViniciusSDias
Copy link
Contributor Author

Are there any actions I need to take here or is this PR good? 😁

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

Successfully merging this pull request may close these issues.

Incorrect error message when passing null to join/implode's array parameter
4 participants