-
Notifications
You must be signed in to change notification settings - Fork 61
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: handle disabled shell_exec #598
Conversation
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
/backport to stable29 |
/backport to stable28 |
'gateway' => '', | ||
]; | ||
|
||
if (function_exists('shell_exec')) { |
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 guess the reason we can't use executeCommand()
here is because of the inherent use of escapeshellcmd()
. Maybe we add an optional parameter to `executeCommand() that specifies whether to escape or not (that defaults true). We could then use consistently (elsewhere too I think) w/o boilerplate.
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.
Yes, escapeshellcmd does not allow a | b
because it escapes the pipe.
I'm not a fan of those piped commands here in serverinfo because of the bad readability. Parsing the command output with PHP is easier to read and test.
I went for the function_exists approach to easily backport it without changing to much.
I have a few ideas in mind for follow-ups
- Move executeCommand to a trait
- Drop dns (because that's much more complicated today than reading /etc/resolv.conf)
Fix #534