Rewind stream when possible

[theofidry]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	none
Documentation	none
License	MIT

One may consume a response body by doing the following:

$response = $client->sendRequest($request);
$response->getBody()->getContents();

When the $response comes from the cache, this no longer works because some StreamFactory implementations, e.g. DiactorosStreamFactory, don't rewind the stream. As a result, the code above will result in an empty string if the response comes from the cache.

[Nyholm]

Thank you for this PR.

Since 1.5.0 of php-http/message no stram factories rewind streams. We discussed and decided that the HTTP client should be responsible of rewinding the stream before the send a HTTP request.

Version 1.5 changed the behavior here when using Zend (not Guzzle). Should we help the user here by rewind the Response's Stream or should we say that the user is responsible for this? (e.g. use ->toStream() instead of ->getContents().)

[joelwurtz]

IMO, cache plugin should not be responsible to rewind stream.

If someone read the stream and know that other lib / class will use the stream, then this code should rewind it after it has read it.

However that's the theory and in practice not everyone follow it so i can understand the need behind this PR.

IMO we should simply create a RewindPlugin that does this job, and then we can put it / use it when we cannot trust another lib / client, WDYT ?

[joelwurtz]

Nevermind my comment i read thing too fast, i think it make sense here to rewind it

[joelwurtz]

if the stream is not seekable then it's not usable, what do you think about throwing an exception if the stream is not rewindable ?

[theofidry]

I can, what kind of exception would you like to throw in this situation? I'm thinking of an InvalidArgumentException, as it would be a configuration issue IMO.

[Nyholm]

If we should thrown an exception we could skip the if ($stream->isSeekable()). Calling rewind on a non-seekable stream will throw a RuntimeException according to PSR7.

[joelwurtz]

👍 however we should still encapsulate it with a Httplug Exception to respect the contract

[Nyholm]

Ping @theofidry Can you change this to

try {
  $stream->rewind();
} catch (\Exception $e) {
   throw new RewindStreamException('foo', 0, $e);
}

And also create that exception?

[theofidry]

will try to do this weekend

[Nyholm]

Thank you for the update. I had some minor comments

[Nyholm]

Is this valid PHP 5?

[theofidry]

definitely not, fixed it

[Nyholm]

The namespace to src/ is Http\Client\Common\Plugin

But lets put this exception in Http\Client\Common\Plugin\Exception

[dbu]

given how few classes we have i think its ok to have the exception in the main namespace. though the result when using this package together client-common will be that there is a lot of classes in this namespace: https://github.com/php-http/client-common/tree/master/src/Plugin

if we ever add exceptions for specific plugins in client-common we are likely to place them in an Exception sub namespace. for that reason i would prefer to also put this exception into a sub namespace Http\Client\Common\Plugin\Exception.

[Nyholm]

Agreed. Putting the exception in Http\Client\Common\Plugin\Exception\RewindStreamException is preferred.

[Nyholm]

Thank you @theofidry. Sorry for the confusion. I will tag a new release shortly.