search

php-http / message

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

Do not rewind streams #72

Closed Nyholm closed 7 years ago

Nyholm commented 7 years ago

Should we rewind streams and resources when they are passed in the factory?

Guzzle does not rewind at all. Slim rewinds resources but not streams. Zend rewinds everything.

This is related to #71

dbu commented 7 years ago

if we don't read from the stream, we also should not rewind it. i am a bit afraid about the side effect if we change behaviour within a minor version. we should probably only do that for version 2 and add notes to the corresponding changelogs.

Nyholm commented 7 years ago

I think we can update this behavior in a patch release. The factories are not interchangeable, which is a bug. What I want to know is: What is our intent?

I agree that we should not touch an existing StreamInterface.

Why do we like to rewind it? What benefits does it give?

joelwurtz commented 7 years ago

Should we rewind streams and resources when they are passed in the factory?

IMO no

We should only rewind when we read something and we know that another piece of code will certainly need to read it to.

Nyholm commented 7 years ago

Thank you for the feedback. Is this PR ready to be merged? This PR is blocking: https://github.com/php-http/multipart-stream-builder/pull/27

sagikazarmark commented 7 years ago

How do http clients behave when they receive a request? Do they rewind the body?

What is our expectation (and current behaviour) in plugins (where we interact with streams the most apart from the client)?

I guess these need to be taken into account as well. Maybe we could test this branch in our integration test suite with different clients.

Nyholm commented 7 years ago

How do http clients behave when they receive a request? Do they rewind the body?

php-http/curl-client: Does not rewind php-http/socket-client: Does rewind Guzzle: Uses __toString() which rewinds

What is our expectation (and current behaviour) in plugins (where we interact with streams the most apart from the client)?

Plugins that handles responses do always rewind the stream. The end user should not get different responses depending on if a plugin (say LogPlugin) is used or not.

I believe it is the client's responsibility to rewind the stream. Why would you ever want to create a stream, seek to 50% and then just send the second half as a HTTP request?

I will send a PR to php-http/curl-client

dbu commented 7 years ago

now with your fix, curl-client does rewind as well

Nyholm commented 7 years ago

Yes, Correct. @sagikazarmark do you want to investigate some more before we merge? Do you think it is a good idea to merge?

sagikazarmark commented 7 years ago

No, if the consensous is that clients mostly rewind, I am fine with that, users probably expect that behaviour anyway.

Also, stream factories will probably never produce a non-seekable stream.

There is one risk though: one might pass in a stream which is read only, non-seekable. How do we handle that case?

Nyholm commented 7 years ago

There is one risk though: one might pass in a stream which is read only, non-seekable. How do we handle that case?

We do not touch streams at all. We just return them. Same with resources. We do not read form them at all, just wrap them in a stream.

sagikazarmark commented 7 years ago

Okay, so I am totally confused now I guess. 😄 Isn't this PR about REWINDING streams? Or about NOT REWINDING them?

Nyholm commented 7 years ago

Sorry for the confusion. I updated the code after https://github.com/php-http/message/pull/72#issuecomment-272903306 but I did not update the title.

sagikazarmark commented 7 years ago

Ah, okay. 😄 I was on the wrong path then for a long time ago.

sagikazarmark commented 7 years ago

Good to go?

Nyholm commented 7 years ago

Yes!