-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Ensure count() is not called on a string #2888
Ensure count() is not called on a string #2888
Conversation
lib/Varien/File/Uploader.php
Outdated
&& (count($file[0]) > 0) | ||
&& (count($file[1]) > 0) | ||
&& isset($file[0]) | ||
&& isset($file[1]) |
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.
is isset()
a good enoug replacement here? If they used count() on a string here, then it was probably meant for the lengts, to avoid empty strings.
Might a is_string() && strlen() > 0
be a better replacement? What other types can we expect here besides strings?
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.
Might a
is_string() && strlen() > 0
Shorter $file[0] !== ''
?
Edit: think this is something rector
would fix (if i remember right)
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.
Good call. It seems like preg_match
can potentially return integers as a match as well according to documentation examples, although some of the docs are contradictory. In the examples there are unquoted integers as matches, but the return type description states the text
that is matched.
Regarding the logic: The 0-index is simply the string to be matched (if a match is present), so checking existence on that should be sufficient. The 1-index will be a match (or not set at all), so I believe checking existence should be sufficient. Although I don't see a guarantee that the match (if present) will necessarily have a length > 0, this is my assumption. In testing I'm unable to match a 'space' character either.
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.
count()
when used on a string pre PHP 8.0, had the same effect of converting the value to an array before calling it. So count('')
would still result in 1
because it's similar to count([''])
.
From the context of the usage, I think the last two conditions are useless and can be removed since for this specific regex if count($file) > 0
, then $file[0]
and $file[1]
are guaranteed to be set (empty string if they are not matched), and by changing it to !== ''
we are changing the behavior.
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.
If they used count() on a string here, then it was probably meant for the lengts, to avoid empty strings.
This would not work. In PHP7 count()
on a string always returned int 1
(not checking for length)
Edit: count(null)
returns int 0
in PHP7, so
From next lines ....
$fileAttributes = $_FILES[$file[0]];
Array key $file[0]
must be string or int. Neither int nor empty string make sense here, so i think @Flyingmana is right with string comparison.
lib/Varien/File/Uploader.php
Outdated
if (count($file) > 0 | ||
&& (count($file[0]) > 0) | ||
&& (count($file[1]) > 0) | ||
&& isset($file[0]) | ||
&& isset($file[1]) |
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 did some tests on
preg_match("/^(.*?)\[(.*?)\]$/", $fileId, $file);`.
$fieldId
is int or null or 'abc', $file
is []
.
$fieldId
is '1[2]', $file
is
array(3) {
[0] => string(4) "1[2]"
[1] => string(1) "1"
[2] => string(1) "2"
}
$fieldId
is '[abc]', $file
is
array(3) {
[0] => string(5) "[abc]"
[1] => string(0) ""
[2] => string(3) "abc"
}
$fieldId
is 'abc[]', $file
is
array(3) {
[0] => string(5) "abc[]"
[1] => string(3) "abc"
[2] => string(0) ""
}
$fieldIs is 'abc[xyz]', $file
is
array(3) {
[0] => string(8) "abc[xyz]"
[1] => string(3) "abc"
[2] => string(3) "xyz"
}
The next statement
array_shift($file);
removes the first element, now $file
has the last 2 elements, both are string.
Given the above, we should look for non-empty $file[1]
and $file[2]
, $file[0]
is irrelevant as it is removed.
if (count($file) > 0 | |
&& (count($file[0]) > 0) | |
&& (count($file[1]) > 0) | |
&& isset($file[0]) | |
&& isset($file[1]) | |
if (count($file) === 3 && $file[1] && $file[2]) { |
Why not simply this? if (preg_match('/^(\w+)\[(\w+)\]$/', $fileId, $file)) { If the statement is true then |
lib/Varien/File/Uploader.php
Outdated
preg_match("/^(.*?)\[(.*?)\]$/", $fileId, $file); | ||
|
||
if (count($file) > 0 | ||
&& (count($file[0]) > 0) | ||
&& (count($file[1]) > 0) | ||
&& isset($file[0]) | ||
&& isset($file[1]) |
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.
@colinmollenhour suggestion is even better:
preg_match("/^(.*?)\[(.*?)\]$/", $fileId, $file); | |
if (count($file) > 0 | |
&& (count($file[0]) > 0) | |
&& (count($file[1]) > 0) | |
&& isset($file[0]) | |
&& isset($file[1]) | |
if (preg_match('/^(\w+)\[(\w+)\]$/', $fileId, $file)) { |
Instead of checking the results of preg_match by counting the length of the string, simply check for the success value. This prevents PHP 8.1 deprecation warnings.
31f75b0
to
3ec2e39
Compare
Code updated. Thanks everyone for the feedback! |
Instead of checking the results of preg_match by counting the length of the string, simply check for the success value. This prevents PHP 8.1 deprecation warnings.
cherry-picked to 20.0 since there were no conflicts |
This PR fixes the PHP 8.1 deprecation warning on
setUploadFileId
by checking the existence ofpreg_match
values usingisset()
rather thancount()
which is now deprecated for string types.