Skip to content

Add buffered stream #52

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 4 commits into from
Aug 7, 2016
Merged

Add buffered stream #52

merged 4 commits into from
Aug 7, 2016

Conversation

joelwurtz
Copy link
Member

@joelwurtz joelwurtz commented Aug 4, 2016

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Related tickets mentioned in #46, replace #49
Documentation
License MIT

This is a replacement for the MemoryCloner, this allow to decorate a not seekable stream with a seekable one.

In order to do that it buffer the underlying stream into a php://temp resource by default (but can also be fully buffered into memory).

It never reads the underlying stream, until it has been tell to do so.

Once this is merged i will add a EnsureSeekableStreamPlugin, which decorates the request body and the response body with this one if the existing stream is not seekable, so we would be able to get the stream content (format) even for not seekable stream (only if user want it).

private $stream;

/** @var int How many bytes were written */
private $writed = 0;
Copy link
Member

Choose a reason for hiding this comment

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

should be named to $written

$this->rewind();

return $this->getContents();
} catch (\Throwable $throwable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this php 7 only? ah if it does not exist, php 5 will ignore this and use the catch below. maybe add a comment below that thats only for BC with php 5 and can be removed when going to php 7

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@joelwurtz
Copy link
Member Author

Is it ok to merge ?

@dbu
Copy link
Contributor

dbu commented Aug 7, 2016

looks good to me. @Nyholm @sagikazarmark do you agree?

@sagikazarmark
Copy link
Member

AFAIK there are some PSR7 compliance test suits out there. Maybe we should use them to test PSR7 implementations. If they are not good enough, maybe we could write one? Apart from that, LGTM.

@Nyholm
Copy link
Member

Nyholm commented Aug 7, 2016

👍

@dbu dbu merged commit ffad01f into master Aug 7, 2016
@dbu dbu deleted the feature/buffered-stream branch August 7, 2016 12:58
@dbu
Copy link
Contributor

dbu commented Aug 7, 2016

more testing is always good, but lets not hold up merging. @joelwurtz if you have time to check out the idea from mark, that would be great to detect issues before we release.

@joelwurtz
Copy link
Member Author

I only found https://github.com/Maks3w/Psr7Assertions but this does not test stream

@sagikazarmark
Copy link
Member

I was once thinking about writting a PSR7 tester lib which accepts message factories to create messages/streams so that it can handle specific data and other options. The other possibility is to have an abstract method which returns a PSR7 object instance and that can be used since its immutable, although there would be some drawbacks like streams are noz immutable, etc.

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.

4 participants