Implement PSR-7 about http HOT 47 CLOSED

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

from http.

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.

from http.

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

from http.

@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.

from http.

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

@clue @WyriHaximus How do you plan to address this?

This is something I've been looking into with @legionth for the past few months and we've made some very good progress so far 👍

With the recent changes in v0.5.0 and the fresh v0.6.0 release, our own Request and Response classes have been adapted to look more like the PSR-7 interfaces. In fact, in the upcoming v0.7.0 we will implement the RequestInterface/ServerRequestInterface and ResponseInterface, as they're perfectly fine even for an async approach 👍

Only its StreamInterface does not quite match with our requirements here, as some(!) of its methods imply synchronous operation. We'll likely prepare a class for this that also implements an interface more suited for streaming operation. Stay tuned, we'll file PRs for this in the upcoming days

from http.

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

from http.

@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.

from http.

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

from http.

@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 :)

from http.

👍

from http.

👍

from http.

👍 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

from http.

@gsouf take a look at #41

from http.

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?

from http.

@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.

from http.

that's a great idea, I like it!

from http.

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.

from http.

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

from http.

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.

from http.

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.

from http.

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

from http.

@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.

from http.

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)

from http.

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.

from http.

@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.

from http.

@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.

from http.

@WyriHaximus what is the progress of psr7 impl?

from http.

@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 ! :)

from http.

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

from http.

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

from http.

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

@clue @WyriHaximus How do you plan to address this?

from http.

@clue we are tunned, you can broadcast :D

from http.

@clue ServerRequestInterface::getUploadedFiles and ServerRequestInterface::getParsedBody aren't compatible either unless you want to buffer the complete request body ahead of time, at which point you don't have issues with StreamInterface either.

I think being kinda compatible but not really is worse than not being compatible at all.

from http.

@kelunik You're making some very valid points here and I assure you we're taking care of this, so I'd suggest we wait for the actual PRs to discuss these things 👍

from http.

I'm really looking forward to your PSR7 ServerRequestInterfaceimplementation. This will make it really easy to throw Slim Framework in the mix!

from http.

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

from http.

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. ;)

from http.

@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?

from http.

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!

from http.

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 👍

from http.

$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!

from http.

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.

from http.

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.

from http.

+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 :(

from http.

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.

from http.

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

from http.

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 👍

from http.