Replace Response class with PSR-7 Response class by legionth · Pull Request #152 · reactphp/http

legionth · 2017-03-26T04:27:46Z

This is big PR. I recommend to try reviewing the single commits here.

The Response class will be replaced with an that implements the PSR-7 ResponseInterface.
So the callback function only needs the request object to be called. A PSR-7 Response object will now be returned. Alternatively a Promise can be returned in this function, but this promise MUST resolve a PSR-7 Response. If the response need time be created, a promise must be used (checkout examples/04-stream-request.php).

The new Response extends from RingCentral\Psr7\Response but adds the possibility to add an ReadstreamInterface implemnation as body.

Refs #28
Builds on top of #146

WyriHaximus

👍 for your work, found some things, any comment that occurs more then once is to point out all occurrences for the given comment.

WyriHaximus · 2017-03-26T11:42:08Z

README.md

-$response->writeHead(200, array(
-    'Date' => date('D, d M Y H:i:s T')
-));
+$server = new Server($socket, function (RequestInterface $request) use ($loop) {


No need for $loop here

WyriHaximus · 2017-03-26T11:42:44Z

README.md

-$response->writeHead(200, array(
-    'Date' => array()
-));
+$server = new Server($socket, function (RequestInterface $request) use ($loop) {


No need for $loop here

WyriHaximus · 2017-03-26T11:42:57Z

README.md

-$response->writeHead(200, array(
-    'X-Powered-By' => 'PHP 3'
-));
+$server = new Server($socket, function (RequestInterface $request) use ($loop) {


No need for $loop here

WyriHaximus · 2017-03-26T11:45:11Z

README.md

-$response->writeHead(200, array(
-    'X-Powered-By' => array()
-));
+$server = new Server($socket, function (RequestInterface $request) use ($loop) {


No need for $loop here

WyriHaximus · 2017-03-26T12:23:39Z

src/Server.php

+            $response = $response->withBody($body);
+        }
+
+        if ($response->hasHeader('Transfer-Encoding')) {


Shouldn't be a second check to ensure the contents of this header is chunked before we run the body through the chunked encoder? In case we add transfer encoding X or Y in the future?

This is necessary at moment because nothing else would be allowed here. But have changed it to make this more ovious.

WyriHaximus · 2017-03-26T12:34:21Z

README.md

 See also [example #3](examples) for more details.

 The constructor is internal, you SHOULD NOT call this yourself.
 The `Server` is responsible for emitting `Request` and `Response` objects.


This line and the 3 lines above it are no longer relevant and ended up in middle of a code example, see https://github.com/legionth/http/blob/c0aef5da52404f458222acf3554c5f591c63395d/README.md#response

WyriHaximus · 2017-03-26T12:36:33Z

README.md

@@ -156,22 +169,39 @@ gives you access to the incoming request body as the individual chunks arrive:

 ```php
 $http = new Server($socket, function (RequestInterface $request, Response $response) {


We don't pass $response anymore since this PR

WyriHaximus · 2017-03-26T12:36:41Z

README.md

@@ -118,9 +126,14 @@ and will be passed to the callback function like this.

 ```php 
 $http = new Server($socket, function (RequestInterface $request, Response $response) {


We don't pass $response anymore since this PR

WyriHaximus · 2017-03-26T12:36:52Z

README.md

@@ -71,8 +76,11 @@ $socket = new React\Socket\SecureServer($socket, $loop, array(
 ));

 $http = new Server($socket, function (RequestInterface $request, Response $response) {


We don't pass $response anymore since this PR

WyriHaximus · 2017-03-26T12:36:58Z

README.md

@@ -53,8 +55,11 @@ constructor with the respective [request](#request) and
 $socket = new React\Socket\Server(8080, $loop);

 $http = new Server($socket, function (RequestInterface $request, Response $response) {


We don't pass $response anymore since this PR

WyriHaximus · 2017-03-26T12:43:17Z

src/Server.php

+use Psr\Http\Message\ResponseInterface;
+use RingCentral;
+use React\Stream\ReadableStream;
+use RingCentral\Psr7\Response;


Shouldn't we be using our own response class here, as it extends that class? Also we have a class name conflict: https://travis-ci.org/reactphp/http/jobs/215140364#L237

Yea, must be a leftover thank you for spotting this.

jsor · 2017-03-26T13:50:13Z

src/Server.php

+            $promise = new Promise(function ($resolve, $reject) use ($promise) {
+                $resolve($promise);
+            });
+        }


These 6 lines can be shortened to:

$promise = \React\Promise\resolve($callback($request));

Additional benefit: supports also foreign thenables.

Another question: What happens if $callback synchronously throws an exception/throwable? This should probably turned into a rejected promises:

try { $promise = \React\Promise\resolve($callback($request)); } catch (\Throwable $e) { $promise = \React\Promise\reject($e); } catch (\Exception $e) { $promise = \React\Promise\reject($e); }

I'd prefer this over just letting the exception bubble up because it would handle exceptions in the same ways as if $callback returns a rejected promises (see also my next comment).

The problem with $promise = \React\Promise\resolve($callback($request)); is that would have to use try-catch. With my lines the Exception would be handled by the Promise and will be recognized as rejected. But thanks for the line didn't knew it ;)

jsor · 2017-03-26T13:56:40Z

src/Server.php

+                }
+                $that->handleResponse($conn, $response, $request->getProtocolVersion());
+            }
+        );


This must also handle a rejected promise returned from $callback.

You are right. Added something to cover this. Have a look 👍

andig · 2017-03-26T16:19:47Z

README.md

+$http = new Server($socket, function (RequestInterface $request) {
+    return new Response(
+        200,
+        array('Content-Type' => 'text/plain')


misses trailing ,
same thing happening below

Thx for spotting this.

clue

Love this feature and I'm happy to get this in once the outstanding change requests are resolved!

WyriHaximus · 2017-03-26T17:50:55Z

@clue yeah can't wait for v0.7.0 to be released and start adapting my work for v0.8.0 🎉 . This, v0.7.0, will be a huge milestone on it's own 👍

legionth · 2017-03-27T22:42:59Z

Thank you for all your rewievs! I rebased the branch. This didn't really changed the code much, but I hope this will make clearer what really changed.

Happy to hear your opinons about this.

clue · 2017-03-27T23:01:18Z

README.md


-The `Response` class is responsible for streaming the outgoing response body.
+The callback function passed to the constructor of the [Server](#server)
+is responsible to return process a response, which will be delivered to the client.


…is responsible for processing the request and returning a response, which…

clue · 2017-03-27T23:02:09Z

README.md

-The `Response` class is responsible for streaming the outgoing response body.
+The callback function passed to the constructor of the [Server](#server)
+is responsible to return process a response, which will be delivered to the client.
+This function MUST return either a implementation of the


…MUST return an instance implementing…

clue · 2017-03-27T23:02:46Z

README.md

+which implements the `PSR-7 RequestInterface` in this project.
+We use instantiation of this class in our projects,
+but feel free to use any implemantation of the 
+`PSR-7 RequestInterface` you prefer.


I would suggest moving this to above the example, what do you think?

clue · 2017-03-27T23:03:09Z

README.md


-The `Response` will automatically use the same HTTP protocol version as the
-corresponding `Request`.
+If your response takes time to be processed you SHOULD use a `ReactPHP Promise`.


Why is this a SHOULD, what happens if consumers ignore this?

clue · 2017-03-27T23:03:43Z

README.md

+This needs to be a promise, because the request body may needs time to be completed.
+The `ReactPHP Promise` will resolve in a `Response` object when the request
+body ends.
+Always use the `ReactPHP Promise` when your 


clue · 2017-03-27T23:05:51Z

README.md

+This is just a example you could use of the streaming,
+you could also send a big amount of data via little chunks 
+or use it for body data that needs to calculated.
+Use the oppertunties of the `ReactPHP Streams`


Last sentence can probably be omitted?

clue · 2017-03-27T23:07:00Z

README.md

-    'Date' => date('D, d M Y H:i:s T')
-));
+$server = new Server($socket, function (RequestInterface $request) {
+    return new \RingCentral\Psr7\Response(200, array('Date' => date('D, d M Y H:i:s T')));


class name?

clue · 2017-03-27T23:07:46Z

README.md

+        200,
+        array(
+            'Content-Type' => 'text/plain',
+            'Content-Length' => strlen($data)


There's room for improvement here 👍 Is this really still necessary? 👍

clue · 2017-03-27T23:10:39Z

README.md

+Make sure to catch these `Exceptions` to create own response messages.
+
+After the return in the callback function the response will be processed by the `Server`.
+The `Server` will add the protocol version of the reequest, so you don't have to.


typo: request

clue · 2017-03-27T23:11:11Z

README.md


+An unhandled Exception in the code of the callback function will result
+in an `500 Internal Server Error` message.
+Make sure to catch these `Exceptions` to create own response messages.


What happens if the callback function returns the wrong value?

jsor · 2017-03-28T06:10:54Z

src/Server.php

+                $that->handleResponse($conn, $response, $request->getProtocolVersion());
+            },
+            function ($ex) use ($that, $conn) {
+                $that->emit('error', array($ex));


Note, that promises can be rejected with a non-exception (there is a pending PR to change that: reactphp/promise#93). The docs suggest a typehint against Exception for the error event listeners.

$http->on('error', function (Exception $e) { });

Maybe check against \Exception/ \Throwable here and turn non-exceptions into a \Exception?

Added code to cover this behaviour.

jsor · 2017-03-28T06:12:52Z

src/Server.php

+use Psr\Http\Message\ResponseInterface;
+use RingCentral;
+use React\Stream\ReadableStream;
+use React\Promise\Promise;


Since you're starting to use react/promise explicitely, i recommend adding it to the composer.json. At the moment react/promise is only indirectly required via react/socket.

Added to react/promise to composer.json

legionth · 2017-03-28T11:15:02Z

Ping @clue . I fixed the README, also found some mistakes myself :) . Have a look.

jsor · 2017-03-28T12:48:00Z

src/Server.php

                $that->handleResponse($conn, $response, $request->getProtocolVersion());
            },
            function ($ex) use ($that, $conn) {
+                if (!$ex instanceof \Exception) {


Should test against \Throwable too.

Keep in mind that the error event is type-hinted as Exception though!

jsor · 2017-03-28T12:55:59Z

src/Server.php

            },
            function ($ex) use ($that, $conn) {
+                if (!$ex instanceof \Exception) {
+                    $ex = new \InvalidArgumentException('Unknown rejection type');


This exception message is very generic. Suggestion:

$ex = new \InvalidArgumentException( sprintf( 'The response callback returned a rejected promise with a reason of type "%s" instead of a \Throwable or \Exception instance.' is_object($ex) ? get_class($ex) : gettype($ex) ) );

jsor · 2017-03-28T12:59:34Z

src/Server.php

+        $promise->then(
+            function ($response) use ($that, $conn, $request) {
+                if (!$response instanceof ResponseInterface) {
+                    $that->emit('error', array(new \InvalidArgumentException('Invalid response type')));


Very generic exception message. Suggestion:

$ex = new \InvalidArgumentException( sprintf( 'The response callback is expected to return an object implementing Psr\Http\Message\ResponseInterface, but returned "%s" instead.' is_object($response) ? get_class($response) : gettype($response) ) );

Added a more useful message. Have a look.

jsor · 2017-03-28T12:59:58Z

src/Server.php

+            },
+            function ($ex) use ($that, $conn) {
+                if (!$ex instanceof \Exception) {
+                    $ex = new \InvalidArgumentException('Unknown rejection type');


Very generic exception message. Suggestion:

$ex = new \InvalidArgumentException( sprintf( 'The response callback returned a rejected promise with a reason of type "%s" instead of a \Throwable or \Exception instance.' is_object($ex) ? get_class($ex) : gettype($ex) ) );

Added a more useful message. Have a look.

jsor · 2017-03-28T13:00:15Z

src/Server.php

+                $that->handleResponse($conn, $response, $request->getProtocolVersion());
+            },
+            function ($ex) use ($that, $conn) {
+                if (!$ex instanceof \Exception) {


Should test against \Throwable too.

Thanks for the review.I reconsidered this and changed the code. The error-event on the Server should always be an Exception.
In the newest changes a RuntimeExpception will always be created, and a thrown exception will be added as previous to this RuntimeException.
Checkout the documentation and the code.

clue · 2017-03-29T06:07:23Z

README.md

@@ -270,49 +353,58 @@ will automatically use chunked transfer encoding and send the respective header
 If you know the length of your body, you MAY specify it like this instead:


Does this paragraph still apply? I see some room for improvement here with the updated API 👍

Yes this does still apply, at least for streams. Changed the documenation, have a look.

clue · 2017-03-29T06:08:16Z

README.md

 ```

 If you do not want to send this header at all, you can use an empty array as
 value like this:


clue · 2017-03-29T06:08:56Z

examples/01-hello-world.php

+    return new Response(
+        200,
+        array(
+            'Content-Length' => strlen("Hello world\n"),


See above comment, is this line really (still) necessary?

You are right. Change it in the newest commit.

clue · 2017-03-29T06:17:58Z

src/Server.php

+
+        if ($response->getHeaderLine('Transfer-Encoding') === 'chunked') {
+            $stream = new ChunkedEncoder($body);
+        }


Isn't this header always removed or am I missing something? Also, who should be in control of this behavior in this first place? This whole method looks unneeded complex to me, sending a constant string body could be simplified, what do you think?

Changed structure

WyriHaximus · 2017-03-29T09:44:22Z

README.md

-    'X-Powered-By' => 'PHP 3'
-));
+$server = new Server($socket, function (RequestInterface $request) {
+    return new Response(200, array('X-Powered-By' => 'PHP 3'));


👍 on using PHP 3 as example, although I would have approved of 6 as well 😆

Well, I can still change it while I'm on it. Just used the same example as before 😅

If it where up to me: go for it 😎

+1 for PHP 6!

clue

Awesome! 🎉

jsor · 2017-03-30T13:44:26Z

src/Server.php

+            function ($error) use ($that, $conn) {
+                $message = 'The response callback is expected to resolve with an object implementing Psr\Http\Message\ResponseInterface, but rejected with "%s" instead.';
+                $message = sprintf($message, is_object($error) ? get_class($error) : gettype($error));
+                $exception = new \RuntimeException($message, null, $error instanceof \Exception ? $error : null);


You missed \Throwable support here, opened #155.

legionth changed the title ~~Replace Response class with PSR-7 Response~~ Replace Response class with PSR-7 Response class Mar 26, 2017

WyriHaximus requested changes Mar 26, 2017

View reviewed changes

jsor requested changes Mar 26, 2017

View reviewed changes

andig reviewed Mar 26, 2017

View reviewed changes

clue reviewed Mar 26, 2017

View reviewed changes

andig mentioned this pull request Mar 26, 2017

react/http 0.7 php-pm/php-pm#225

Closed

legionth force-pushed the handle-psr7-responses branch 3 times, most recently from 57cc76c to fd1affb Compare March 27, 2017 22:31

clue reviewed Mar 27, 2017

View reviewed changes

jsor requested changes Mar 28, 2017

View reviewed changes

legionth force-pushed the handle-psr7-responses branch from fd1affb to b71e67b Compare March 28, 2017 10:51

jsor requested changes Mar 28, 2017

View reviewed changes

clue reviewed Mar 29, 2017

View reviewed changes

legionth force-pushed the handle-psr7-responses branch from b71e67b to afc1bcd Compare March 29, 2017 22:14

WyriHaximus approved these changes Mar 30, 2017

View reviewed changes

Return Promise with PSR-7 Response resolving

1892782

legionth force-pushed the handle-psr7-responses branch 2 times, most recently from 8b954bc to 64839a4 Compare March 30, 2017 11:56

jsor approved these changes Mar 30, 2017

View reviewed changes

legionth added 3 commits March 30, 2017 14:51

Handle Exception in callback function

909b06c

Adapt examples to always returning a promise

bc4e9bc

Update README

ce27ec6

legionth force-pushed the handle-psr7-responses branch from 64839a4 to ce27ec6 Compare March 30, 2017 12:51

clue approved these changes Mar 30, 2017

View reviewed changes

clue merged commit e734181 into reactphp:master Mar 30, 2017

clue added this to the v0.7.0 milestone Mar 30, 2017

clue added the new feature label Mar 30, 2017

jsor mentioned this pull request Mar 30, 2017

Support \Throwable when setting previous exception from server callback #155

Merged

jsor reviewed Mar 30, 2017

View reviewed changes

This was referenced Mar 30, 2017

Responses to HEAD requests and certain status codes never contain a body #156

Merged

Support asterisk-form request target for OPTIONS method #157

Merged

Support CONNECT method #158

Merged

		@@ -156,22 +169,39 @@ gives you access to the incoming request body as the individual chunks arrive:

		```php
		$http = new Server($socket, function (RequestInterface $request, Response $response) {

		@@ -118,9 +126,14 @@ and will be passed to the callback function like this.

		```php
		$http = new Server($socket, function (RequestInterface $request, Response $response) {

		@@ -71,8 +76,11 @@ $socket = new React\Socket\SecureServer($socket, $loop, array(
		));

		$http = new Server($socket, function (RequestInterface $request, Response $response) {

		@@ -53,8 +55,11 @@ constructor with the respective [request](#request) and
		$socket = new React\Socket\Server(8080, $loop);

		$http = new Server($socket, function (RequestInterface $request, Response $response) {

		@@ -270,49 +353,58 @@ will automatically use chunked transfer encoding and send the respective header
		If you know the length of your body, you MAY specify it like this instead:

Uh oh!

Conversation

legionth commented Mar 26, 2017 • edited by clue Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WyriHaximus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andig Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

WyriHaximus commented Mar 26, 2017

Uh oh!

legionth commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsor Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

legionth commented Mar 26, 2017 •

edited by clue

Loading

andig Mar 26, 2017 •

edited

Loading

jsor Mar 28, 2017 •

edited

Loading