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.

Sign up for GitHub

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

Jump to bottom

Emit non seekable streams #2803

Merged
merged 3 commits into from Aug 15, 2019
Merged

Emit non seekable streams #2803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b68b1fd
Fix incorrect emit non-seekable streams
mapogolions Aug 15, 2019
d10ac81
Add tests for emit different stream types
mapogolions Aug 15, 2019
a023af2
Make changes to legacy prophecy objects
mapogolions Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions Slim/ResponseEmitter.php
View file
Open in desktop
Expand Up @@ -133,7 +133,11 @@ public function isResponseEmpty(ResponseInterface $response): bool
if (in_array($response->getStatusCode(), [204, 205, 304], true)) {
return true;
}
$contents = (string) $response->getBody();
return !strlen($contents);
$stream = $response->getBody();
$seekable = $stream->isSeekable();
if ($seekable) {
$stream->rewind();
}
return $seekable ? $stream->read(1) === '' : $stream->eof();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be made easier with ... ?

return $stream->getSize() > 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
2 changes: 2 additions & 0 deletions tests/AppTest.php
View file
Open in desktop
Expand Up @@ -1564,6 +1564,7 @@ public function testRun()
$body .= $args[0];
$this->__toString()->willReturn($body);
});
$streamProphecy->read(1)->willReturn('_');
$streamProphecy->read('11')->will(function () {
$this->eof()->willReturn(true);
return $this->reveal()->__toString();
Expand Down Expand Up @@ -1616,6 +1617,7 @@ public function testRunWithoutPassingInServerRequest()
$body .= $args[0];
$this->__toString()->willReturn($body);
});
$streamProphecy->read(1)->willReturn('_');
$streamProphecy->read('11')->will(function () {
$this->eof()->willReturn(true);
return $this->reveal()->__toString();
Expand Down
33 changes: 30 additions & 3 deletions tests/ResponseEmitterTest.php
View file
Open in desktop
Expand Up @@ -38,7 +38,7 @@ public function testRespond()
$this->expectOutputString('Hello');
}

public function testRespondNoContent()
public function testResposeWithNoContentSkipsContentTypeAndContentLength()
{
$response = $this
->createResponse()
Expand All @@ -55,7 +55,7 @@ public function testRespondNoContent()
$this->expectOutputString('');
}

public function testNonEmptyResponse()
public function testNonEmptyResponseDoesNotSkipContentTypeAndContentLength()
{
$response = $this
->createResponse()
Expand Down Expand Up @@ -197,7 +197,34 @@ public function testIsResponseEmptyWithNonEmptyBodyAndTriggeringStatusCode()
$this->assertTrue($responseEmitter->isResponseEmpty($response));
}

public function testAvoidReadFromSlowStreamAccordingStatus()
public function testIsResponseEmptyDoesNotReadAllDataFromNonEmptySeekableResponse()
{
$body = $this->createStream('Hello');
$response = $this
->createResponse(200)
->withBody($body);
$responseEmitter = new ResponseEmitter();

$responseEmitter->isResponseEmpty($response);

$this->assertTrue($body->isSeekable());
$this->assertFalse($body->eof());
}

public function testIsResponseEmptyDoesNotDrainNonSeekableResponseWithContent()
{
$resource = popen('echo 12', 'r');
$body = $this->getStreamFactory()->createStreamFromResource($resource);
$response = $this->createResponse(200)->withBody($body);
$responseEmitter = new ResponseEmitter();

$responseEmitter->isResponseEmpty($response);

$this->assertFalse($body->isSeekable());
$this->assertSame('12', trim((string) $body));
}

public function testAvoidReadFromSlowStreamAccordingToStatus()
{
$body = new SlowPokeStream();
$response = $this
Expand Down