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

Emit non seekable streams #2803

Merged
merged 3 commits into from Aug 15, 2019

Conversation

@mapogolions
Copy link
Contributor

commented Aug 15, 2019

Every time when we emit any response we read all data from a stream twice.

There may be problems with slow streams or responses with too big body. In addition, with a such ResponseEmitter work, it is impossible to send a response with non seekable body.

@coveralls

This comment has been minimized.

Copy link

commented Aug 15, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling a023af2 on mapogolions:fix/emit-non-seekable-stream into a1ca5e9 on slimphp:4.x.

if ($seekable) {
$stream->rewind();
}
return $seekable ? $stream->read(1) === '' : $stream->eof();

This comment has been minimized.

Copy link
@shadowhand

shadowhand Aug 15, 2019

Contributor

Could this be made easier with ... ?

return $stream->getSize() > 0;

This comment has been minimized.

Copy link
@mapogolions

mapogolions Aug 15, 2019

Author Contributor

Your logic condition need to invert. return !$stream->getSize() > 0. Please, run unit tests with your code. You will see some errors (AppTest errors can be ignored). The most interesting is the error Slim\Tests\ResponseEmitterTest::testRespondIndeterminateLength. It shows that reading from a stream of at least one byte is the most reliable way. I myself don’t like such excessive complexity.

This comment has been minimized.

Copy link
@mapogolions

mapogolions Aug 15, 2019

Author Contributor

I found yet another example. Resource $resource = popen('echo 12', 'r') will be defined as empty, but in reality has content.

@l0gicgate l0gicgate requested a review from adriansuter Aug 15, 2019

@l0gicgate l0gicgate added this to the 4.2.0 milestone Aug 15, 2019

@l0gicgate l0gicgate added the Slim 4 label Aug 15, 2019

@l0gicgate l0gicgate merged commit 86a22f7 into slimphp:4.x Aug 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@mapogolions mapogolions deleted the mapogolions:fix/emit-non-seekable-stream branch Aug 15, 2019

@l0gicgate l0gicgate referenced this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.