SapiEmitter::emitBody() suppresses exceptions by using StreamInterface::__toString()

[jesseschalken]

SapiEmitter::emitBody() is currently:

echo $response->getBody();

The definition of StreamInterface::__toString() specifies:

This method MUST NOT raise an exception in order to conform with PHP's string casting operations.

and Zend\Diactoros\Stream::__toString() indeed suppresses run-time exceptions:

try {
    $this->rewind();
    return $this->getContents();
} catch (RuntimeException $e) {
    return '';
}

GuzzleHttp\Psr7\Stream::__toString() even suppresses all exceptions:

try {
    $this->seek(0);
    return (string) stream_get_contents($this->stream);
} catch (\Exception $e) {
    return '';
}

As a practical result of this, whenever there is an exception in the process of reading the stream, instead of it bubbling up to the next matching catch block or to the exception handler to notify the developer, the HTTP client gets an empty response instead. In other words, an error which the developer could have been notified of directly via their error reporting system is instead translated into something the user must report manually. I don't want to wait for our users to submit bugs when they see a white page. I want errors going to our error reporting system wherever possible (currently Bugsnag) so we know about it immediately.

I don't know what "conform with PHP's string casting operations means", but I did find this:

Johannes explained that there is no way to ensure that an exception thrown during a cast to string would be handled correctly by the Zend Engine, and that this won't change unless large parts of the Engine are rewritten.

So it seems to me, __toString() in PHP is basically broken when it comes to exceptions. Either you allow it to throw exceptions, in which case they don't bubble and you get a fatal error instead, or you suppress the exceptions and turn them into a broken result which the user has to report. Neither are ideal.

I therefore propose that SapiEmitter::emitBody() not depend on __toString() and instead be:

$stream = $response->getBody();
if ($stream->isSeekable()) {
    $stream->rewind();
}
echo $stream->getContents();

Do you think this would be a good idea?

[jesseschalken]

It just occurred to me - why is emitBody() converting the stream to a string in the first place? Isn't the whole point of a stream that it can be streamed?

$stream = $response->getBody();
if ($stream->isSeekable()) {
    $stream->rewind();
}
while (!$stream->eof()) {
    echo $stream->read(1048576);
}

[weierophinney]

I performed a ton of benchmarks when developing psr-7 that indicated that
when dumping the entire stream at once, the implementation in
__toString() tends to be most performant for payloads falling within
average web page and API payload sizes. You can always create alternate
emitters if your needs are different.
On Jun 21, 2016 9:39 PM, "Jesse Schalken" [email protected] wrote:

It just occurred to me - why is emitBody() converting the stream to a
string in the first place? Isn't the whole point a stream that it can be
streamed?

$stream = $response->getBody();if ($stream->isSeekable()) { $stream->rewind();}while (!$stream->eof()) { echo $stream->read(1048576);}

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#181 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABlV7UxnPQGXcCCXOnCGJD4pr8XxuOoks5qOKBGgaJpZM4I6cvi
.

[jesseschalken]

Doesn't everybody "need" their exceptions not to be ignored? Isn't that why they throw exceptions?

[Venorcis]

Moreover, toString always returns an empty string when isSeekable is false due to the unchecked call to rewind there. I don't think toString should be used in SapiEmitter either.

[weierophinney]

This repository has been closed and moved to laminas/laminas-diactoros; a new issue has been opened at laminas/laminas-diactoros#20.