-
Notifications
You must be signed in to change notification settings - Fork 152
Fix #357 - invalid characters in path/query #372
Conversation
@weierophinney I was looking for right solution of this issue, and I think I have found one finally. |
return $string; | ||
} | ||
|
||
$letters = str_split($string); |
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.
Noting here, because I had to look it up - str_split()
splits into bytes instead of characters when it encounters a multibyte string.
*/ | ||
private function filterInvalidUtf8(string $string) : string | ||
{ | ||
if (preg_match('//u', $string)) { |
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 don't understand what this is doing, exactly.
I've tried it locally with a variety of strings from your query provider below, and every single one of them results in a positive match, which means the logic below never gets hit. Maybe you're not testing invalid UTF-8 characters anywhere?
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.
How come?
var_dump(preg_match('//u', "\x21\x92"));
var_dump(preg_match('//u', "\x21"));
var_dump(preg_match('//u', "\x92"));
result:
bool(false)
int(1)
bool(false)
Somehow tests I've added work.
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.
Turned out I was using single quotes instead of double quotes when I was doing my tests here. Once I changed the quoting style, I was able to observe the same behaviour finally.
Might be good to drop a note in indicating what this is doing, though, so future contributors/maintainers know the purpose.
Forward port #372 Conflicts: CHANGELOG.md
tx for fix! |
Fixes #357
Fixes #218