search

Nyholm / psr7

A super lightweight PSR-7 implementation
MIT License
1.16k stars 75 forks source link

creating a stream from string has unexpected behaviour #177

Closed dbu closed 3 years ago

dbu commented 3 years ago

when creating a stream from a string, the resource should be rewinded.

fix #176

Nyholm commented 3 years ago

Thank you.

See #176 for discussion.

dbu commented 3 years ago

thanks!

Zegnat commented 3 years ago

@Nyholm with this merged, should the next release be a major version bump per semver? Or do you want to run this as a bug fix only? It has the potential to break people’s implementations if they were building on top of what was previously a stable API from this library.

dbu commented 3 years ago

the only scenario for this to fail is when you create a stream with a non-empty string and rely on the stream being empty because it is not rewinded. is there any use case where this could matter?

maybe theoretically if you create the stream with a string but then want to append to the stream you now overwrite the string? no idea what people do and if this could happen, but maybe it can?

Nyholm commented 3 years ago

I think the same principal should apply.

You should never assume where the read pointer is. That is not part of any BC promise.

previously a stable API from this library.

That is true, but I am arguing that this never was a part of the API. The correct way is to always place your pointer after you read/write from a stream.

But we could maybe be more explicit about this by releasing a minor version instead and with a clear note in the changelog. What do you think?

Zegnat commented 3 years ago

You should never assume where the read pointer is. That is not part of any BC promise.

My argument would be that Stream::create() is part of the nyholm/psr7 API, even if not defined by PSR. The output of Stream::create('Hello') can no longer be chained with ->write(' world!') without the result being different. This to me means we have broken the BC promise with this PR.

If only the PSR methods are seen as included in the promise (or in the “public API” as semver says), it can easily be argued that the promise specifically does not mention the pointer. This would just be a patch or minor release to bring us inline with another part of the ecosystem. But if our own static factory methods are part of the promise my hardliner stance would be that a major version increase is required per strict semver.

(Alternatively both can be true, and one can argue the library never stated it was following strict semver.)

I am not sure I have a strong opinion about this, if we accept that we change the library to match the ecosystem we can just change it, but I know some people might have higher expectancies of semver.

Nyholm commented 3 years ago

My argument would be that Stream::create() is part of the nyholm/psr7 API, even if not defined by PSR

True!

The output of Stream::create('Hello') can no longer be chained with ->write(' world!') without the result being different. This to me means we have broken the BC promise with this PR.

Hm... Im not 100% convinced my point is valid. But we never specified this behaviour and it was untested..

I do think we should follow semver and I do think we should avoid doing a new major release.

Currently we annoy the people that expect the stream to be rewinded, if we release this patch, we will annoy all other people... Hm..