Improve preg_* functions warnings for NUL byte #13068
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Girgias
reviewed
Jan 3, 2024
ext/pcre/tests/preg_grep_error1.phpt
Outdated
| '/[a-zA-Z]/', //Regex string | ||
| ]; | ||
| $array = [123, 'abc', 'test']; | ||
| foreach ($values as $value) { | ||
| @print "\nArg value is $value\n"; |
There was a problem hiding this comment.
Why is the print call suppressed here?
There was a problem hiding this comment.
Otherwise we get Warning: Array to string conversion in. What do you think about removing this print, as this is not really necessary?
There was a problem hiding this comment.
Oh it's because of that nested array... I mean this will break in PHP 9 when we promote this sooooooo, removing it is probably fine
There was a problem hiding this comment.
Thank you for review. I removed unnecessary strings from those tests such:
- header of a test printed
valueprintDonestring at the end of test
| $subject = 'test'; | ||
| foreach($regex_array as $regex_value) { | ||
| foreach ($regex_array as $regex_value) { | ||
| @print "\nArg value is $regex_value\n"; |
| $subject = 'this is a test'; | ||
| foreach($regex_array as $regex_value) { | ||
| foreach ($regex_array as $regex_value) { | ||
| @print "\nArg value is $regex_value\n"; |
| function integer_word($matches) { | ||
| global $replacement; | ||
| return $replacement[$matches[0]]; | ||
| } | ||
| $subject = 'number 1.'; | ||
| foreach($regex_array as $regex_value) { | ||
| foreach ($regex_array as $regex_value) { | ||
| @print "\nArg value is $regex_value\n"; |
| $replace = 1; | ||
| $subject = 'a'; | ||
| foreach($regex_array as $regex_value) { | ||
| foreach ($regex_array as $regex_value) { | ||
| @print "\nArg value is $regex_value\n"; |
| $subject = '1 2 a 3 4 b 5 6'; | ||
| foreach($regex_array as $regex_value) { | ||
| foreach ($regex_array as $regex_value) { | ||
| @print "\nArg value is $regex_value\n"; |
Girgias
reviewed
Jan 6, 2024
| @@ -6,11 +6,9 @@ Test preg_match() function : error conditions - wrong arg types | |||
| /* | |||
| * Testing how preg_match reacts to being passed the wrong type of subject argument | |||
| */ | |||
There was a problem hiding this comment.
This test seems to be a ZPP test, and should be removed.
Girgias
reviewed
Jan 6, 2024
| @@ -8,28 +8,21 @@ Test preg_match_all() function : error conditions - wrong arg types | |||
| /* | |||
There was a problem hiding this comment.
This test also looks like a ZPP test
Girgias
approved these changes
Jan 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I thought that replacing
NULtoNUL bytebrings more clarity what we expect as delimiter inpreg_*functions.It can be replaced with
null character,null terminator, or maybe with explicitNUL (\0), but simpleNULcan be mistaken with thenullin PHP language. I was also confused when I was seeing this message for the first time.Additionally I formatted code of tests which were affected by this change.