icicleio / http

HTTP component for Icicle
MIT License
61 stars 5 forks source link

Response required ReadableStreamInterface #3

Closed mtymek closed 9 years ago

mtymek commented 9 years ago

Icicle\Http\Message\Response requires ReadableStreamInterface in its constructor:

public function __construct(
    $code = 200,
    array $headers = null,
    ReadableStreamInterface $stream = null,
    $reason = null,
    $protocol = '1.1'
) {
    parent::__construct($headers, $stream, $protocol);

    $this->status = $this->validateStatusCode($code);
    $this->reason = $this->filterReason($reason);
}

Is there a reason for that? I would say, it should be WritableStreamInterface, but it wouldn't match with abstract Message class. Solution would be to move constructor away from Message. What do you think?

trowski commented 9 years ago

Both Request and Response require a ReadableStreamInterface object because the stream needs to be readable at a minimum in order for the object to be useful. MessageInterface::getBody() returns a ReadableStreamInterface object for this same reason. Server reads the stream from the Response object and pipes it to the client socket stream.

Yes, often a DuplexStreamInterface object is provided so the stream may be written to by the application, but writing to the stream isn't strictly necessary. Perhaps you'd provide a read-only file stream (which is a component that still needs to be written) as the stream in a Response object.

Also consider a Response object in the client context (which is also a part of this library). In this case the response object would be read by the application code.

Is there a problem you've run into when trying to use these objects? Maybe I could help. Sorry there isn't documentation yet. My time has been occupied by other work.

mtymek commented 9 years ago

No, I don't have any problems, it's just that I don't like how it currently works for handling server-side requests. Say we have a middleware that takes a requests, fills response with data and then sends it back to client. What would be the best, elegant way to ensure that response is writable, without adding IF instruction within middleware itself? I fully agree with what you wrote above, but how should we handle my use case?

trowski commented 9 years ago

My understanding is that such middleware would return a new response object with a different stream of it's choosing. Since the messages are immutable, middleware really should be returning a new instance rather than writing to the stream of another object. Does that make sense, am I understanding middleware correctly?

mtymek commented 9 years ago

Yeah, it makes sense. Actually, after getting into coding, I had to change my approach, so this is not a problem anymore.

trowski commented 9 years ago

I would be interested in seeing what you're doing. I'm debating if I should switch up some things about Server, so I'd like to have some more input. I wanted to get this package relatively stable so I could build the WebSocket package on top of it soon.

mtymek commented 9 years ago

I'm mostly playing around now ;-) It is related to Psr7Bridge, I'm trying to build a stack of PSR-7 middlewares that can be easily connected to Icicle.

At first I wanted to convert Icicle's request into PSR7 message, pass it along, take the response, convert it, and send back to user. When working on it, I thought that Response body should always be writable, so I filled this ticket :) But, it was a dead end, it didn't work as expected, complicated things and looked ugly. So now I'm going to skip converting response in favour of writting emitter that takes PSR-7 message and sends it via Icicle. After getting to something interesting I'll publish my results to Gist or a repo.

What changes are you considering? If you want my opinion, open an issue and @-ping me :-)