Implement PSR-7

[mkusher]

I think it would be useful to implement PSR-7 Request\Response. Is it possible?

[WyriHaximus]

I'm currently going over PSR-7 to see how and if we can implement it.

[franzliedke]

👍

[romainPrignon]

👍

[andig]

PSR7 would be absolutely great if it would allow us to skip some of the complex bridging logic e.g. between React and Symfony Request/Responses.

[arunpoudel]

It would be a great thing to have, but they kind of derped on this one, I don't think there is a way around it using async features.

[franzliedke]

@arunpoudel Can you expand on the derp? Are you referring to the immutable structures?

[WyriHaximus]

@franzliedke I think @arunpoudel is referring to the streams which are going to be an issue to fully and properly implement them to spec. My strategy right now is to get around reading HTTP RFC's for 1.0/1.1/2.0 and PSR-7 to get a good and global plan for all of those. But PSR7 might come sooner then the rest.

[letharion]

I came to this issue thinking precisely what @andig wrote above. Lower the limit to interoperability between Symfony and React would be awesome. (And I guess that's the precise idea with PSR-7)

[cboden]

Thinking out loud:

$http->on('request', function ($conn, RequestInterface $request, ResponseInterface $response) {
    $response = $response->withStatus(200)->withHeader('foo', 'bar');

    $conn->writeHead($response);
});

We pass a connection object that has today's React Response methods but it accepts PSR-7 objects for its calls. We'd have to look into see if we can async stream a body with their StreamInterface as well. $response would be a bare or minimalist initialized response for the user to build up.

[WyriHaximus]

@cboden working on PSR7 to ReactPHP streams and vice versa for my guzzle adapters anyway. It is possible but it isn't the prettiest thing around. Will ping you when I have them done.

[andig]

@cboden, @WyriHaximus I've successfully used Guzzle's StreamWrapper to convert PSR7 or older interfaces into native PHP streams (for use with a JsonStreamingParser). This way I'm already able to work with Symfony's StreamedResponse.

To finish implementation of https://github.com/php-pm/php-pm-httpkernel/blob/master/Bridges/HttpKernel.php#L155 I'd be interested in passing any kind of async object stream etc back into ReactPHP. If I can help/ test please let me know.

[ephrin]

@WyriHaximus what is the progress of psr7 impl?

[gsouf]

👍 I currently cant work with this project, because I need port and host data from request uri that are available with PSR7 request, but not with the built in request object

[WyriHaximus]

@gsouf take a look at #41

[gsouf]

@WyriHaximus, pointed my ref on the latest commit from this branch while waiting for version 0.4.2, thanks.

However PSR7 is still very desired for ease of use with other libraries ! :)

[WyriHaximus]

@gsouf keep in mind that PSR7 isn't fully async compatible, although IIRC @clue has something for that somewhere

[sandrokeil]

This would be really awesome. You can read more about that in the blog post Serve PSR-7 Middleware Via React by @weierophinney.

[clue]

To post just a small status update: This is something I'm currently looking into with @legionth, so stay tuned.. 😎

[legionth]

@danielnitz this is the next on my list. This will be definitely in v0.7.0.

[danielnitz]

@legionth if I can help by testing, let me know!

[stefanotorresi]

so, related to my last commend on #146 , here are a few things to note, being the current implementation not actually compatible with PSR-7.

Let's assume I want to use a PSR-15 middleware pipe and serve them with react/http:

use Psr\Http\Message\ServerRequestInterface;
use Interop\Http\ServerMiddleware\DelegateInterface;
use Interop\Http\ServerMiddleware\MiddlewareInterface;
use Zend\Diactoros\Response\TextResponse;

$pipe = new class implements MiddlewareInterface {
    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        return new TextResponse('foobar', 200);
    }
};
$delegate = new class implements DelegateInterface {
    public function process(ServerRequestInterface $request) { }
};

// blablabla instantiate loop, the socket, etc

$server = new React\Http\Server($socket, function ($request) use ($pipe, $delegate) {
    try {
        $response = $pipe->process($request, $delegate);
    } catch (Throwable $t) {
        $response = new TextResponse((string) $t, 500);
    }
    return $response;
});

// blablabla run the loop, log errors, etc

This doesn't work for a number of reasons:

• The $request instance is not a Psr\Http\Message\ServerRequestInterface but a Psr\Http\Message\RequestInterface. But I get that @legionth is already working on this, so no problem, as long as this makes to the 0.7.0 release (which is the milestone for PSR-7 readiness).
• With the current implementation of Psr\Http\Message\StreamInterface, the $pipe->process($request, ...) is gonna fail hard with anything that operates on the request body, because most of the stream operations are not actually supported.
• Returning a response implementation different from React\Http\Response doesn't appear to work. Nothing happens. (haven't had the time to investigate on this one)

I hope this issues are resolved before 0.7.0. I'll do what I can to help, maybe I can wrap up some integration tests in another PR, but for the time being I just wanted these issues to be acknowledged. ;)

[clue]

@stefanotorresi Thanks for digging into this and providing us with valuable feedback! 👍

With the current implementation of Psr\Http\Message\StreamInterface, the $pipe->process($request, ...) is gonna fail hard with anything that operates on the request body, because most of the stream operations are not actually supported.

The current documentation explicitly warns against this because the $request will be emitted before any of the body is even available. The documentation suggest either taking advantage of this by using a streaming approach in your code or buffering the request stream so that the "normal" PSR-7 semantics apply again. I hope this helps? 👍

Returning a response implementation different from React\Http\Response doesn't appear to work. Nothing happens. (haven't had the time to investigate on this one)

This sounds like unexpected behavior, please let us know if you dig into this and find anything! 👍 Afaict, this simply calls __toString() on the body as soon as the callback resolves with the $response, so you may have to re-arrange your code to only resolve once your body is actually complete?

[kelunik]

Afaict, this simply calls __toString() on the body as soon as the callback resolves with the $response, so you may have to re-arrange your code to only resolve once your body is actually complete?

If it does that, how is streaming the response going to work?

[stefanotorresi]

The current documentation explicitly warns against this because the $request will be emitted before any of the body is even available. The documentation suggest either taking advantage of this by using a streaming approach in your code or buffering the request stream so that the "normal" PSR-7 semantics apply again. I hope this helps?

Yes, as I said on IRC I understand the technical implications, but IMHO this kinda defeats the point of implementing PSR-7: if in react/http you need to treat request and response bodies differenty, I can't see how other projects should interoperate with it without bridges and adapters!

[maciejmrozinski]

@stefanotorresi I think that all of this can be simplified as:

• if You want to use asynchronous streaming, then it wont be fully compatible with PSR7 implementations (middlewares, etc) since PSR7 is synchronous by design
• if You wait for whole request to be received (as mentioned by @clue ) and create response with body (this should work, maybe there is some bug), then all PSR7 implementations will be compatible

Even simpler: if You want full PSR7, don't use async streaming for request and response. Correct me if I messed up anything.

[clue]

If it does that, how is streaming the response going to work?

Have you even looked at the readme? :p

treat request and response bodies differenty

You don't? See the readme, from within react/http they are always streaming bodies, but you can use buffering to get complete bodies as per PSR-7.

@maciejmrozinski I think this sums it up quite nicely 👍

Our solution does not aim for 100% of use cases. However, most use cases should be able to easily adapt to the given design and live with an 80% solution.

Let's face it, if you really need full PSR-7 support, you're gonna have a hard time with a streaming approach. Keep in mind that this project is still beta and this may change in the future. However, I'm still genuinely curious which use-cases are really affected by this in the first place 👍

[franzliedke]

@maciejmrozinski Can you explain how async streaming for the request body would be disabled when using react/http? And when doing so, would that automatically result in a fully PSR-7 compatible stream instances being used?

IMO, not following a standard is better than following it incompletely. If things are incompatible, then so be it. IMO, it would be harming the standard, when certain things about that standard could not be relied upon anymore.

[WyriHaximus]

In my personal opinion 0.7 will bring low level streaming PSR-7 support that isn't supposed to be used in higher level applications. It is ment for websocket servers, or other programs that rely on stream in the request and stream out the response for whatever their reason is.

However, 0.8 will bring body parsers which can turn a body stream into a fully PSR-7 compatibleparsed server request. Those requests can be used in higher level applications. We could be using a BufferStream to give users access to a request body once it has been parsed. The details aren't set in stone yet and we're open for feedback on it. We aim to provide a simple to use tool to take care of that:

$http = new Server($socket, function (RequestInterface $request) {
    return Magic::vodoo($request)->then(function (RequestInterface $request) {
        return $application->run($request);
    });
});

This will cover both the scenarios @maciejmrozinski mentions and will be extensively be documented and examples will be added.

[maciejmrozinski]

@stefanotorresi Your issues with Zend/Diactoros Response implementation should be fixed by #164

[stefanotorresi]

$http = new Server($socket, function (RequestInterface $request) {
    return Magic::vodoo($request)->then(function (RequestInterface $request) {
        return $application->run($request);
    });
});

@WyriHaximus this is the crux of the question: why does the outer request passed to the server callback need to be a half-baked PSR-7 one? I'd rather just have a full PSR-7 with the buffered body in the inner callback.

If we need Magic:voodoo anyway, why bother with a not fully compatible implementation?

@clue you talk about the "80%" use cases. Could you make some examples?
The way I see it, being PSR-7 inherently synchronous as you rightfully noted, the main use case for a PSR-7 implementation of react/http is to switch from an asynchronous context to a synchronous one (you asked for use cases, but I already gave you one: feed the server request into an pre-existing synchronous application).
It seems that your 80% of use cases, instead, is forcing an incomplete PSR-7 implementation into the async context, and I fail to understand what's the value that PSR-7 brings with this, since you're gonna end up using react/http specific features anyway because, as you noted yourself, you're gonna have a hard time with a streaming approach with PSR-7. Then again, I've only started working with ReactPHP a few months ago, so I'm probably missing something.

@maciejmrozinski I can confirm it now works as expected! Thanks!

[franzliedke]

I agree with @stefanotorresi.

Maybe mapping React request to PSR-7 requests belongs to projects like php-pm, which are trying to make "classical" synchronous apps work well together with React anyways.

[andig]

What @franzliedke is writing is in line with my experience. At php-pm we've successfully been using react/http as long as the body parsers where existing. So did phly/react2psr7. The main point of compatibility is the parsers, not the psr7 interface imho. On the other hand side I do not see why the current approach to 0.7 should be harmful. It's just not and probably will not be a 100% solution for making react/http compatible with psr7 synchronous use cases.

[ssipos90]

+1 for creating the StreamInterface (implementation) instance when we have the whole request buffered.

Perhaps a buffer class that could work something like:

class StreamBuffer {
    // add the constructor receiving the Request

    public function promise(){
        return new Promise(function ($resolve, $reject) {
            // buffer here
            // ...
            $this->request->getBody()->on('end', function () use ($resolve) {
                $resolve(new Stream($this->buffer)); // implements StreamInterface
            });
        });
    }
}

which would plug in something like:

$server = new HttpServer($socket, function ($request) {
    // add other listeners here
    return (new StreamBuffer($request))->promise();
});

The buffer class doesn't have to deal with the promise, but a "plug and play" system for stuff like this is nice to have.

Edit: this idea is bad for multipart :(

[WyriHaximus]

@WyriHaximus this is the crux of the question: why does the outer request passed to the server callback need to be a half-baked PSR-7 one?

@stefanotorresi Because when dealing with a lot of large requests memory usage will spike. Because when building a websocket server on react/http a buffered request is completely useless.

I'd rather just have a full PSR-7 with the buffered body in the inner callback.

How about we provide a server class for both usecases, a StreamingServer and a BufferedServer. The former, StreamingServer, is for websocket servers and others who want to have more control over the request/response streams and call Magic:voodoo when they desire. The latter, BufferedServer, uses is a thin wrapper around StreamingServer that calls Magic:voodoo and once resolved it calls the callable handed to the server with a fully compatible PSR-7 request.

This will make the decision which server you're using very conscious about what kind of request you get passed into your callable.

[stefanotorresi]

that's a great idea, I like it!

[kelunik]

@WyriHaximus But then the StreamingServer shouldn't use PSR-7 IMO.

[stefanotorresi]

@stefanotorresi Because when dealing with a lot of large requests memory usage will spike. Because when building a websocket server on react/http a buffered request is completely useless.

exactly, so a PSR-7 one is completely useless :)

[kelunik]

Regarding WebSockets: They don't work with any such request object. The handshake is fine with PSR-7, as it must not contain a body. What's required for WebSockets is a socket exporter to use the raw socket resource.

[clue]

Thanks for the elaborate discussion so far guys! 👍 I would like to thank everybody involved for raising their ideas and also concerns and can assure you we're taking all of this very serious and that we value and consider every input.

That being said, keep in mind that this is the crux of software engineering: There are no silver bullets.

This means that we do our best to find solutions that fit best for our target audience and that we have to find compromises which ultimately won't be able to fit 100% of use cases.

I think @WyriHaximus did a very good job of describing what a future API could look like. Rest assured we WILL provide an implementation that brings full PSR-7 support! However, this will required access to the parsed body data etc. and as such is something that is left up for the v0.8.0 release via #105 and referenced issues. I you would like to discuss this further, I would like to ask you to comment on this ticket instead.

This ticket here focuses on bringing our APIs in line with PSR-7 for the the v0.7.0 release. Given that we have a number of use-cases that involve streaming, this also includes streaming support among PSR-7 support. This implies that this intermediary release may not implement full PSR-7 support.

I personally don't see this being an issue given that it already covers a relevant number of use cases (see the README, examples and also linked issues for more details) and despite its limitations (also documented in the README) we have yet to come up with any major issues here.

I understand that this may perhaps not cover 100% of uses cases as there may be a number of use cases which may require full PSR-7 support. If your use case is not covered by the intermediary v0.7.0 release, I would suggest waiting for the next v0.8.0 release (or later releases, such as v1.0.0). Given that these likely rely on #105, I would like to ask you to comment on this ticket instead or opening new ticket if you feel your issue is not covered elsewhere.

[andig]

Seems PSR-7 is pretty much finished as far as 0.7 goes, see https://github.com/reactphp/http/milestone/12

[clue]

Thanks for everybody involved 👍 See all related tickets that have been linked against this issue and have been closed in the meantime.

These changes will be part of the v0.7.0 release that is due in the next days

I'll assume this is resolved and will close this for now, please feel free to report back otherwise 👍