-
Notifications
You must be signed in to change notification settings - Fork 40
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
Added cURL formatter #50
Conversation
|
||
$body = $request->getBody()->__toString(); | ||
if (!empty($body)) { | ||
$command .= sprintf(' --data \'%s\'', $body); |
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.
Maybe we should espace the body (what give a json string with ' or " inside ?, need a test for this)
$body = $request->getBody(); | ||
if ($body->getSize() > 0) { | ||
if (!$body->isSeekable()) { | ||
throw new \RuntimeException('Cannot take data from a stream that is not seekable'); |
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.
Sending an exception here is horrible, i.e. some requests can randomly fail only on debugging (not on prod) in a symfony env with this formatter in the debug toolbar. Can we set a specific exception like : CannotFormatException (or something else) that we can catch in class using this formatter to not show something ?
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 was planning to remove this code when your request cloner is merged.
did you look at https://github.com/namshi/cuzzle ? if so we should probably mention the authors of that in the class docblock. is there anything in namshi that we are missing here? |
I did not. Haven't seen this package until now. They have somethings we dont.. I'll update the PR. |
I've update the PR with some fixes borrowed from Cuzzle. See the diff. It is not too much code. Should I mention the Cuzzle authors? |
Just mention in the README in a credits section that it is inspired by cuzzle. |
thanks a lot! created php-http/documentation#136 about documenting this. we might also want to enable it in HttplugBundle |
Thank you for merging. 👍 |
if (!$body->isSeekable()) { | ||
return 'Cant format Request as cUrl command if body stream is not seekable.'; | ||
} | ||
$command .= sprintf(' --data %s', escapeshellarg($body->__toString())); |
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 wonder if we should have some sort of sanity check to not put huge files in here? say if the body size > 5000 we truncate or just do something like [too large body] in the command? if we use this formatter eg in the symfony debug toolbar, it could break on large files
Thanks a lot @Nyholm |
What's in this PR?
This will add a cURL formatter