Revert #177

[Nyholm]

This will revert #177 so we don't introduce new behaviour. This will fix #180.

FYI @Zegnat @dbu. The discussion in #180 stalled a bit. By reverting this we are predictable (but maybe wrong), we are also consistent between creating a stream from string compared to from resource.

[dbu]

imho this is overthinking it.

if you really think its better to have it this way, i suggest to explain why the behaviour is this way in the StreamTest::testBuildFromString.

and to clarify the behaviour in the documentation.

the readme example on $psr17Factory->createStream should have the rewind and a note why you need to rewind.

and the phpdoc for Stream::create and for the psr17 factory createStream methods should also have a warning that you need to rewind the stream you just created if you want it to work.

[Nyholm]

I agree with you that your way would make more sense.

In #180 we discussed that this change would be a BC break because there might be application that will break if this was released.

What is your opinion on the BC issue?

[dbu]

What is your opinion on the BC issue?

thats what i meant with "overthinking" it. i consider rewinding the stream a DX fix that can have side effects if somebody relied on undocumented behaviour. but i can't really claim it is a bug fix either, as the behaviour is unspecified in the spec, so it is not strictly a bug.

if it was a straight up bug, i think BC considerations would not make sense, but with this situation it is really an edge case. maybe we should simply release a new major if there is too much worry about side effects?

[dbu]

in the end its your call. if i was the author of the library, i would consider it a fix and accept the risk that it could be a problem in an edge case... but it your library, so do what you think is right ;-)

[Zegnat]

Like I said in the previous discussion: I do consider this a semver BC break, but have no real opinion on how strictly we are supposed to be following semver. I am very much of the opinion that not documenting the stream cursor position has been a big miss on the PSR mapping of PHP streams into PSR-7/PSR-17, and maybe when I have the time to do so I will try to reignite the discussion on that in the mailing list again.

I wonder if reverting this and then planning for a 2.0 release might be a good plan, where we specifically make the goal of 2.0 to compare with default behaviours of other PSR-7 implementations? #181 comes to mind as another one where the spec may be a slightly ambiguous and following convention might be a good idea. I would just like to see the data that there actually exists consensus amongst implementations :)

[Nyholm]

I really do want to avoid a version 2.0 of this library.

Could we make sure the (soon to be released) Guzzlehttp/psr7:2.0 is aligned with us instead?

I do consider this a semver BC break, but have no real opinion on how strictly we are supposed to be following semver.

Has this "feature" ever been documented? It sure has not been tested. If it has not been tested or documented, is it really a feature?

[Zegnat]

Has this "feature" ever been documented? It sure has not been tested.

Surely the fact that the PR changes a test, means it is tested? 😉

But yeah, as said in the original discussion, hard to say. Honestly this library does not make any other promises than PSR does. And with that said, PSR makes no promises about the cursor ever (as far as I can recall right now). So in some way, all behaviour around the cursor is undocumented behaviour. And the only way to ensure Stream is ever right is to rewind after the fact, to use __toString() to get all contents, etc.

Annoylingly enough, projects following pure PSR defined behaviour would never notice this difference. Because they should be using __toString() and manually managing cursor. It is only projects depending on the undocumented behaviour of PSR-7 that notice the difference between this library and others...

Could we make sure the (soon to be released) Guzzlehttp/psr7:2.0 is aligned with us instead?

Have they answered the cursor question? Has it been documented why certain decisions were reached?

I would describe our behaviour to be:

• Creating a Stream from a string is an alias for fopen on a block of memory, writing to it, and then using that resource as stream.

To me it sounds like the alternative is:

• Creating a Stream from a string is an alias for creating a file with the string in it, fopen the file, and then using that resource as stream.

It is clear from PSR-7 that Steam object matches closely to the functions around fopen. I do not know what the most obvious way is to get the underlying stream resource when one needs to be created from a string. I personally think our way is very intuitive.

[Nyholm]

We may be overthinking. But not merging this PR might lead to more head aches.

I'll marge this and then tell users to never use Stream::getContents().