Skip to content

Imply UTF8 validity in implode function #10780

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
Merged
merged 3 commits into from
Mar 7, 2023
Merged

Imply UTF8 validity in implode function #10780

merged 3 commits into from
Mar 7, 2023

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Mar 4, 2023

improve #10463

@mvorisek mvorisek marked this pull request as draft March 4, 2023 11:50
@mvorisek mvorisek force-pushed the implode_utf8_valid branch from 5d68416 to 3f9aab5 Compare March 4, 2023 13:48
@mvorisek mvorisek force-pushed the implode_utf8_valid branch from 3f9aab5 to fa3a358 Compare March 4, 2023 13:58
@mvorisek mvorisek marked this pull request as ready for review March 4, 2023 13:58
mvorisek
mvorisek commented Mar 4, 2023
View reviewed changes
@mvorisek mvorisek force-pushed the implode_utf8_valid branch from fa3a358 to 587ae73 Compare March 4, 2023 15:26
@nielsdos
Copy link
Member

nielsdos commented Mar 4, 2023

I think doing this for implode makes sense, and the implode changes look correct. As for the dup case: I don't know if we should do that, if code that uses dup modifies the string and is not prepared for flag handling this could cause issues.

@mvorisek
Copy link
Contributor Author

mvorisek commented Mar 4, 2023 •
edited
Loading

As for the dup case: I don't know if we should do that, if code that uses dup modifies the string and is not prepared for flag handling this could cause issues.

makes sense, thank you, reverted at least from this PR

nielsdos
nielsdos approved these changes Mar 5, 2023
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

I believe your changes make sense and are correct.
I'll keep this open for a few more days to see if any other feedback comes in.
If no feedback comes in anymore, I'll merge it in a few days.
Thank you for your contribution!

Girgias
Girgias reviewed Mar 6, 2023
View reviewed changes
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.

Looking at the special handling of int types (which looks like some sort of optimization) I'm expecting: var_dump(implode("a", ["HELLO", 5, "WORLD", 9, "WTF"])); to not be marked as a UTF-8 string.

I think the easiest way would be to yank the special IS_LONG handling away and just always go through the else branch.

@nielsdos nielsdos merged commit 3821938 into php:master Mar 7, 2023
@mvorisek mvorisek deleted the implode_utf8_valid branch March 7, 2023 18:31
@mvorisek mvorisek mentioned this pull request Mar 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

@Girgias Girgias Girgias left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@mvorisek @nielsdos @Girgias