-
Notifications
You must be signed in to change notification settings - Fork 39
Bug/curl fromater fails #94
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
Conversation
I create PR, but I have several variants of task solve, but I don’t know which is needed. |
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.
Great. Thank you.
We may also want to check if $data
is empty before we try to escape it.
$command .= sprintf(' --data %s', escapeshellarg($data)); | ||
|
||
$escapedData = @escapeshellarg($data); | ||
if (isset($php_errormsg)) { |
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.
Wouldn't this always be false? Nothing defines $php_errormsg
, right?
|
||
$escapedData = @escapeshellarg($data); | ||
if (isset($php_errormsg)) { | ||
throw new \InvalidArgumentException($php_errormsg); |
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.
We do not want any exceptions or errors here. We want to silently fail. If we want to be extra nice to the user we may include a error message in the command.
// if fail:
$escapedData = 'We cound not escape the data properly';
escapeshallarg failed
@@ -45,7 +45,11 @@ public function formatRequest(RequestInterface $request) | |||
} else { | |||
$data = '[non-seekable stream omitted]'; | |||
} | |||
$command .= sprintf(' --data %s', escapeshellarg($data)); | |||
|
|||
$escapedData = @escapeshellarg($data) or |
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.
Lets make this more pretty by doing multiple lines.
$escapedData = @escapeshellarg($data);
if (empty($escapedData)) {
$escapedData = 'We couldn't not escape the data properly';
}
Any updates on this PR? |
@Nyholm I'll do it asap |
Thanks, you good for me, poke @Nyholm |
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.
Looks good. I missed the notification for your last commit.
Good job!
What's in this PR?
Added suppress to CurlFormatter for big file error.