search

php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.3k stars 42 forks source link

Rewind streams in the formatter #46

Closed Nyholm closed 8 years ago

Nyholm commented 8 years ago
Q A
Bug? yes
New Feature? no
Version 1.3.0

Actual Behavior

From: https://github.com/php-http/HttplugBundle/pull/84#issuecomment-232911288

Just see the formatter and how the request / response is read.

As far as i have see, this formatter mess up with the stream, as __toString is called and the stream is not rewind so the content is retrieve only in the first stream ?

(I may have miss something again ^^)

Expected Behavior

Streams should be rewinded after they are read.

joelwurtz commented 8 years ago

Rewinding the stream is not hard, IF the stream is rewindable :)

If not the current design is broken as we have to replace the stream in the request passed to the formatter but since the request is immutable we cannot update the stream easily.

Then there is some work to do in the plugin calling the formatter which holds the responsability of rewinding / replacing the stream.

joelwurtz commented 8 years ago

Also cloning / rewinding the stream is something that it's used in the cache plugin, maybe we can refactor this part to have an helper stream / class doing this ?

Nyholm commented 8 years ago

👍 For a helper class doing this. I did the same hack in an other PR: https://github.com/KnpLabs/php-github-api/pull/389/files#diff-192f7f95a475a8e6e67b97d1c5125367R22

Not too sure how to solve the broken design though... Maybe do not read the stream if it not is rewindable/seekable?

sagikazarmark commented 8 years ago

Or copy the stream.

dbu commented 8 years ago

Or copy the stream.

we should try to avoid that. the memory impact can be massive. i think we should by default leave the body alone, especially if its not rewindable, and probably have 3 options: don't show body, show body if stream can rewind, always show body (and copy streams if needed)

Nyholm commented 8 years ago

We cant copy the string because the Request is immutable.

I've created a PR.