Fix of stream saving

[Ryllaz]

Using rewind for correct stream reading - you can get empty body in stream_get_contents

[Ryllaz]

The problem is in the case of multiple requests. For example, if you need use OAuth you need do several requests. And if you're using php://temp stream you need rewind for every request because second call of stream_get_contents will return empty body.

[Zegnat]

I am not sure what behaviour this is fixing. PSR-7 defines two ways to read from a Stream instance: cast to string, or call the getContents-method. I believe both are correctly implemented in this lib. Could you add a unit test showing what it is that is fixed by this PR?

If you have code using PSR-7 Streams for some form of output, is there some reason you cannot cast to string to always get the full contents? Wondering if it is a behavioural error on the part of this implementation, or if it is just the fact that it is not super clear that PSR-7 has two ways to get Stream contents.

Note that this PR breaks backwards compatibility and will neccessitate a major version bump for semver. As Stream instances will have the output of a number of methods changed (e.g. tell() and getContents()).

[jaapromijn]

We have encountered an issue after upgrading a project. The body we send to an API is suddenly empty. Debugging it lead to this class (Stream) and stumbled upon this issue. I don't have extra context, but just wanted to say that we encountered it too :)

Also I tried the code in this PR and that indeed fixed it; the request has a body again.

[Ryllaz]

I discovered the possible ways of getting response body.

First way:

...
$result = (string)$response->getBody()->getContents(); // resut string will NOT be empty

Second way:

...
$body = $response->getBody();
$body->rewind();
$result = $body->getContents(); // resut string will NOT be empty

The problem is that if you try to read the stream data without rewind but just with getContents() - at the start reading process current cursor's position is just after the last byte of the data just written. And stream_get_contents starts reading from the end of data to the end of the stream.

That's why result is empty.

This PR should fix this problem.

Also I think that php://temp stream should be closed just after receiving of the response - I can not imagine any correct cases to use it again without reopen. Thats why I hope that this PR will not break the logic in projects it will be used in.

[sunkan]

@Zegnat I did a bit of research and the other popular psr-7/psr-17 library all do the rewind when creating the temporary resource

• laminas/laminas-diactoros
• slim/psr7
• guzzlehttp/psr7

And here is the most basic failing test

$this->assertTrue(Stream::create('test string')->getContents() === 'test string'); // at the moment this fails

Also this change is not really connected to psr-7 it tries to fix a problem with the psr-17 factory

It's just that this library have the factory part on the Stream class

And sure the specification doesn't specify if you should rewind the stream when creating it or not

[Zegnat]

Also this change is not really connected to psr-7 it tries to fix a problem with the psr-17 factory

Very true. But what I am questioning is it being a problem 😉 PSR-7 does not give any guarantees that getContents() reads from the start of the stream content. For interoperability, if someone is consuming a PSR-7 object, they should never assume that is does. You either rewind(), check tell(), or cast to string if you do want to get the full stream contents. Those are the only things where PSR-7 guarantees interoperability for.

I did a bit of research and the other popular psr-7/psr-17 library

I could have saved you some time, I already did this research, with 13 different PSR implementations, back in 2018. It was almost a 50/50 split in results. There is a full discussion on the PHP-FIG mailing list started by me, a PR under discussion to ammend PSR-17 by another contributor, and a PR to the interoperability unit tests to check for cursor positions also by me.

Back in late 2018 / start 2019 we decided not to update the Nyholm/psr7 library because there was no consensus and we were hoping to get the PSR ammended instead. We would then follow wherever the PSR goes. Which would introduce a breaking change and would mean a version 2.0.0 release on our end.

Maybe I need to repeat the research and see if libraries are more in favour of one solution over the other.

Wondering if we should document this somewhere…

---

But to reitterate: if you need to know the full contents of a Stream instance, you should never assume getContents() gives it to you. That is not what PSR-7 says. Any middleware could have changed the cursor position, no matter which implementation you are using. So for correct interop work, please always rewind it yourself 😊

[sunkan]

I would be fine with the idea of just updating the documentation.

I remember being bitten by this when change implementation a couple of years ago.

After that I use __toString() when I need the entire content because the documentation around what it does is clear.

[Nyholm]

Hm. @Zegnat and I have been spending so much time telling people these exact arguments. PSR-7 nor PSR-17 does not specify this and you should never assume where the read pointer is.

I think we can save us some time (and the users the headache) to just be pragmatic instead.

I'll merge #177 because it has tests and a changelog entry.

Thank you @Ryllaz