Repository navigation
Implement PSR-7 #28
Description
Activity
I'm currently going over PSR-7 to see how and if we can implement it.
👍
Reacted by Greg Bowler👍
Reacted by Greg BowlerPSR7 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.
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.
@arunpoudel Can you expand on the derp? Are you referring to the immutable structures?
@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.
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.
$responsewould be a bare or minimalist initialized response for the user to build up.@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.
@cboden, @WyriHaximus I've successfully used Guzzle's
StreamWrapperto convert PSR7 or older interfaces into native PHP streams (for use with aJsonStreamingParser). This way I'm already able to work with Symfony'sStreamedResponse.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.
@WyriHaximus what is the progress of psr7 impl?
👍 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
Reacted by samizdam31 remaining items
In my personal opinion
0.7will 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.8will 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 aBufferStreamto 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.
Reacted by Christian Lück, Maciej Mroziński and danielnitzReacted by Franz Liedke@stefanotorresi Your issues with Zend/Diactoros Response implementation should be fixed by #164
Reacted by Christian Lück, Stefano Torresi and Cees-Jan Kiewiet$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:voodooanyway, 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 ofreact/httpis 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 usingreact/httpspecific 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!
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.
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.
+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 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/httpa 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
StreamingServerand aBufferedServer. The former,StreamingServer, is for websocket servers and others who want to have more control over the request/response streams and callMagic:voodoowhen they desire. The latter,BufferedServer, uses is a thin wrapper aroundStreamingServerthat callsMagic:voodooand 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.
Reacted by Stefano Torresi and Christian LückReacted by Christian Lückthat's a great idea, I like it!
Reacted by Christian Lück@WyriHaximus But then the StreamingServer shouldn't use PSR-7 IMO.
Reacted by Stefano TorresiReacted by Christian Lück@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 :)
Reacted by Christian Lück and andigRegarding 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.
Reacted by Christian LückThanks 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.0release 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.0release. 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.0release, I would suggest waiting for the nextv0.8.0release (or later releases, such asv1.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.Seems PSR-7 is pretty much finished as far as 0.7 goes, see https://github.com/reactphp/http/milestone/12
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 👍
I think it would be useful to implement PSR-7 Request\Response. Is it possible?