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

Add always seekable body plugin #167

Merged
merged 3 commits into from
Feb 23, 2019
Merged

Add always seekable body plugin #167

merged 3 commits into from
Feb 23, 2019

Conversation

joelwurtz
Copy link
Member

@joelwurtz joelwurtz commented Jan 21, 2019

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #98, replace #99, partially #97
Documentation php-http/documentation#251
License MIT

What's in this PR?

Add a plugin that ensure request body or response body is always seekable

Why?

This may be needed when multiple librairies need to acces the content but are not aware of each other, so this allow them to read the body once and then rewind it.

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.

very cool! i have a comment on the testing, and a minor spelling nitpick on the phpdoc

spec/Plugin/AlwaysSeekableBodyPluginSpec.php Outdated Show resolved Hide resolved
src/Plugin/AlwaysSeekableBodyPlugin.php Outdated Show resolved Hide resolved
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.

Good.

Do I want to decide if this should make only the response seekable?

@dbu
Copy link
Contributor

dbu commented Jan 25, 2019

hm, good question. it could make sense to distinguish that, because i assume adding the buffer is a significant cost that should be avoided when its unnecessary. rather than having flags, i would prefer to split the plugins into SeekableRequestBodyPlugin and SeekableResponseBodyPlugin

(either way i think we can drop the "Always" from the name, it is more verbose without adding much more clarity imho)

@joelwurtz
Copy link
Member Author

joelwurtz commented Jan 29, 2019

As you wish, we can also spli this into 2 plugins : RésponseBodySeekable and RequestBodySeekable, which may be better since you will not put this 2 plugins at the same place :

  • RequestBodySeekable should be in first so following plugins can seek its body
  • ResponseBodySeekable should be in last so previous plugins will correctly have a seekable body on the response

@dbu
Copy link
Contributor

dbu commented Jan 29, 2019

didn't even think of that, but yes, lets split the plugin then.

@joelwurtz
Copy link
Member Author

Should be good, will update the documentation accordingly if this is good to merge

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.

looks good to me!

@dbu dbu merged commit 9a2a4c1 into master Feb 23, 2019
@dbu dbu deleted the feature/seekable-stream branch February 23, 2019 18:27
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.

FilteredStream: unable to read content of stream multiple times
3 participants