Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add end method to ChunkedStreamDecoder for compatibility with Response #62

Closed
wants to merge 1 commit into from

Conversation

WyriHaximus
Copy link
Member

Fixes #61

@clue
Copy link
Member

clue commented Sep 23, 2016

👎 This class implements the ReadableStreamInterface, while the end() method is only defined on the WritableStreamInterface. I'm aware of #61, but why is this method called in the first place?

@WyriHaximus
Copy link
Member Author

end() is called because the stream coming into Response is a DuplexStreamInterface and close() kills off the response with an exception.

Another option to this issue is to check if $this->stream implements WritableStreamInterface before calling end().

@clue
Copy link
Member

clue commented Sep 23, 2016

I've just checked the Response class, afaict it actually violates the stream interface by calling end() within its close() method, so I think this should be fixed.

Also, afaict it doesn't really make sense this expects a DuplexStreamInterface in the first place, as it's only using its read-only side anyway. We should be able to change this to a ReadableStreamInterface without causing a BC break (Liskov substitution principle).

I'd vote for closing this one to keep this discussion and vote for somebody to file a new PR that fixes this in the Response instead 👍

@WyriHaximus
Copy link
Member Author

That makes even more sense, I'll close this PR and file a new PR shortly with the suggested change 👍 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants