Skip to content

Improve formatting of requests with binary streams #92

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

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

z38
Copy link
Contributor

@z38 z38 commented Jan 22, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #91
Documentation -
License MIT

What's in this PR?

This PR fixes the handling of null bytes in CurlCommandFormatter. Up to now, an exception is thrown in escapeshellarg. In addition, non-seekable streams get formatted in the same manner.

Example Usage

Requests containing null bytes are formatted as follows:

curl 'http://foo.com/bar' --request POST --data '[binary stream omitted]'

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

To Do

  • At the moment, as soon as any control character (including \n) is detected, the payload is omitted. If we want to keep them we would probably have to implement platform-dependent escaping routines to output a one-liner. escapeshellarg() doesn't help as it would just pass all control characters (except \0). An example of such routines can be found in Chromium.
  • Should we limit the size of the body too? I think it's rarely useful to dump megabytes of data even if there are no null bytes.

@z38 z38 changed the title Improve formatting of requests with binary streams [WIP] Improve formatting of requests with binary streams Jan 22, 2018
@z38 z38 changed the title [WIP] Improve formatting of requests with binary streams Improve formatting of requests with binary streams Jan 22, 2018
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! looks good to me.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thank you

@Nyholm Nyholm merged commit c7f6a70 into php-http:master Jan 22, 2018
@z38 z38 deleted the binary-body branch January 24, 2018 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to format binary payload as command
3 participants