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
Fix possible error on PHP 8.1 #18913
Conversation
A few test failures here but seem unrelated. |
@@ -288,6 +288,11 @@ private static function returnsSuccessCode($command) | |||
{ | |||
$exec = $command . ' > /dev/null 2>&1; echo $?'; | |||
$returnCode = @shell_exec($exec); | |||
|
|||
if (false === $returnCode || null === $returnCode) { | |||
return false; |
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.
It just came to my mind that this will actually change the current behavior. But maybe this might actually fix some possible issues.
shell_exec
returns false
or null
if something didn't work with the command, otherwise the return code is returned.
If an error occurred and e.g. null
or false
was returned, the trim
triggered a warning on PHP 7 and maybe even PHP 8. The error should only occur on PHP 8.1.
After the trim $returnCode
is an empty string in case of an error and 0 == (int) $returnCode
actually evaluates to true
and we assume the command succeeded, which it actually didn't.
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.
@sgiehl not sure I understand it changes behaviour. Looks like this maybe worked before? https://3v4l.org/7C8Qf
Looks generally good though 👍
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.
previously if $returnCode
was false
or null
, this function would return true
, now it returns false
.
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 think this is ok to merge. This change in behaviour would almost certainly be better if it affected anyone at all
Description:
fixes #18910
Review