Skip to content
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

Rewind streams in the formatter #46

Closed
Nyholm opened this issue Jul 15, 2016 · 6 comments · Fixed by #47
Closed

Rewind streams in the formatter #46

Nyholm opened this issue Jul 15, 2016 · 6 comments · Fixed by #47
Assignees

Comments

@Nyholm
Copy link
Member

Nyholm commented Jul 15, 2016

Q A
Bug? yes
New Feature? no
Version 1.3.0

Actual Behavior

From: php-http/HttplugBundle#84 (comment)

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

Nyholm commented Jul 15, 2016

👍 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
Copy link
Member

Or copy the stream.

@dbu
Copy link
Contributor

dbu commented Jul 15, 2016 via email

Nyholm added a commit that referenced this issue Jul 15, 2016
Nyholm added a commit to Nyholm/message that referenced this issue Jul 15, 2016
Nyholm added a commit to Nyholm/message that referenced this issue Jul 15, 2016
@Nyholm Nyholm mentioned this issue Jul 15, 2016
@Nyholm
Copy link
Member Author

Nyholm commented Jul 15, 2016

We cant copy the string because the Request is immutable.

I've created a PR.

sagikazarmark pushed a commit that referenced this issue Jul 15, 2016
* Make sure we rewind stream if we can.
This will fix #46

* style fix

* Updated changelog with fixes for FullHttpMessageFormatter
@joelwurtz joelwurtz mentioned this issue Jul 22, 2016
3 tasks
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 a pull request may close this issue.

4 participants