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

No need for $loop here

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

README.md

No need for $loop here

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

README.md

No need for $loop here

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

README.md

No need for $loop here

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

src/Server.php

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

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

We don't pass $response anymore since this PR

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

README.md

We don't pass $response anymore since this PR

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

README.md

We don't pass $response anymore since this PR

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

README.md

We don't pass $response anymore since this PR

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

src/Server.php

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

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

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

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

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

README.md

…MUST return an instance implementing…

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

README.md

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

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

README.md

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

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

README.md

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

README.md

Last sentence can probably be omitted?

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

README.md

class name?

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

README.md

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

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

README.md

typo: request

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

README.md

What happens if the callback function returns the wrong value?

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

src/Server.php

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

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

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

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

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

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

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

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

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