-
-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace Response class with PSR-7 Response class #152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for your work, found some things, any comment that occurs more then once is to point out all occurrences for the given comment.
README.md
Outdated
$response->writeHead(200, array( | ||
'Date' => date('D, d M Y H:i:s T') | ||
)); | ||
$server = new Server($socket, function (RequestInterface $request) use ($loop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for $loop
here
README.md
Outdated
$response->writeHead(200, array( | ||
'Date' => array() | ||
)); | ||
$server = new Server($socket, function (RequestInterface $request) use ($loop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for $loop
here
README.md
Outdated
$response->writeHead(200, array( | ||
'X-Powered-By' => 'PHP 3' | ||
)); | ||
$server = new Server($socket, function (RequestInterface $request) use ($loop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for $loop
here
README.md
Outdated
$response->writeHead(200, array( | ||
'X-Powered-By' => array() | ||
)); | ||
$server = new Server($socket, function (RequestInterface $request) use ($loop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for $loop
here
src/Server.php
Outdated
$response = $response->withBody($body); | ||
} | ||
|
||
if ($response->hasHeader('Transfer-Encoding')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary at moment because nothing else would be allowed here. But have changed it to make this more ovious.
README.md
Outdated
```php | ||
$http = new Server($socket, function (RequestInterface $request, Response $response) { | ||
$body = "The method of the request is: " . $request->getMethod(); | ||
$body .= "The requested path is: " . $request->getUri()->getPath(); | ||
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
README.md
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass $response
anymore since this PR
README.md
Outdated
@@ -118,9 +126,14 @@ and will be passed to the callback function like this. | |||
|
|||
```php | |||
$http = new Server($socket, function (RequestInterface $request, Response $response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass $response
anymore since this PR
README.md
Outdated
@@ -71,8 +76,11 @@ $socket = new React\Socket\SecureServer($socket, $loop, array( | |||
)); | |||
|
|||
$http = new Server($socket, function (RequestInterface $request, Response $response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass $response
anymore since this PR
README.md
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass $response
anymore since this PR
src/Server.php
Outdated
use Psr\Http\Message\ResponseInterface; | ||
use RingCentral; | ||
use React\Stream\ReadableStream; | ||
use RingCentral\Psr7\Response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, must be a leftover thank you for spotting this.
src/Server.php
Outdated
$promise = new Promise(function ($resolve, $reject) use ($promise) { | ||
$resolve($promise); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
src/Server.php
Outdated
} | ||
$that->handleResponse($conn, $response, $request->getProtocolVersion()); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must also handle a rejected promise returned from $callback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Added something to cover this. Have a look 👍
README.md
Outdated
$http = new Server($socket, function (RequestInterface $request) { | ||
return new Response( | ||
200, | ||
array('Content-Type' => 'text/plain') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
misses trailing ,
same thing happening below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for spotting this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this feature and I'm happy to get this in once the outstanding change requests are resolved!
@clue yeah can't wait for |
57cc76c
to
fd1affb
Compare
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. |
README.md
Outdated
}); | ||
``` | ||
|
||
### Response | ||
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…is responsible for processing the request and returning a response, which…
README.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…MUST return an instance implementing…
README.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest moving this to above the example, what do you think?
README.md
Outdated
|
||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a SHOULD, what happens if consumers ignore this?
README.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…when?
README.md
Outdated
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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last sentence can probably be omitted?
README.md
Outdated
'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'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class name?
README.md
Outdated
200, | ||
array( | ||
'Content-Type' => 'text/plain', | ||
'Content-Length' => strlen($data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's room for improvement here 👍 Is this really still necessary? 👍
README.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: request
README.md
Outdated
``` | ||
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the callback function returns the wrong value?
src/Server.php
Outdated
$that->handleResponse($conn, $response, $request->getProtocolVersion()); | ||
}, | ||
function ($ex) use ($that, $conn) { | ||
$that->emit('error', array($ex)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added code to cover this behaviour.
use Psr\Http\Message\ResponseInterface; | ||
use RingCentral; | ||
use React\Stream\ReadableStream; | ||
use React\Promise\Promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to react/promise
to composer.json
fd1affb
to
b71e67b
Compare
Ping @clue . I fixed the README, also found some mistakes myself :) . Have a look. |
src/Server.php
Outdated
@@ -240,6 +242,9 @@ function ($response) use ($that, $conn, $request) { | |||
$that->handleResponse($conn, $response, $request->getProtocolVersion()); | |||
}, | |||
function ($ex) use ($that, $conn) { | |||
if (!$ex instanceof \Exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should test against \Throwable
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that the error
event is type-hinted as Exception
though!
src/Server.php
Outdated
@@ -240,6 +242,9 @@ function ($response) use ($that, $conn, $request) { | |||
$that->handleResponse($conn, $response, $request->getProtocolVersion()); | |||
}, | |||
function ($ex) use ($that, $conn) { | |||
if (!$ex instanceof \Exception) { | |||
$ex = new \InvalidArgumentException('Unknown rejection type'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
)
);
src/Server.php
Outdated
$promise->then( | ||
function ($response) use ($that, $conn, $request) { | ||
if (!$response instanceof ResponseInterface) { | ||
$that->emit('error', array(new \InvalidArgumentException('Invalid response type'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a more useful message. Have a look.
src/Server.php
Outdated
}, | ||
function ($ex) use ($that, $conn) { | ||
if (!$ex instanceof \Exception) { | ||
$ex = new \InvalidArgumentException('Unknown rejection type'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a more useful message. Have a look.
src/Server.php
Outdated
$that->handleResponse($conn, $response, $request->getProtocolVersion()); | ||
}, | ||
function ($ex) use ($that, $conn) { | ||
if (!$ex instanceof \Exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should test against \Throwable
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
README.md
Outdated
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this paragraph still apply? I see some room for improvement here with the updated API 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this does still apply, at least for streams. Changed the documenation, have a look.
README.md
Outdated
)); | ||
$server = new Server($socket, function (RequestInterface $request) { | ||
return new Response(200, array('X-Powered-By' => 'PHP 3')); | ||
}); | ||
``` | ||
|
||
If you do not want to send this header at all, you can use an empty array as | ||
value like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array?
examples/01-hello-world.php
Outdated
return new Response( | ||
200, | ||
array( | ||
'Content-Length' => strlen("Hello world\n"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment, is this line really (still) necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Change it in the newest commit.
src/Server.php
Outdated
|
||
if ($response->getHeaderLine('Transfer-Encoding') === 'chunked') { | ||
$stream = new ChunkedEncoder($body); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed structure
b71e67b
to
afc1bcd
Compare
'X-Powered-By' => 'PHP 3' | ||
)); | ||
$server = new Server($socket, function (RequestInterface $request) { | ||
return new Response(200, array('X-Powered-By' => 'PHP 3')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on using PHP 3 as example, although I would have approved of 6 as well 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I can still change it while I'm on it. Just used the same example as before 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it where up to me: go for it 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for PHP 6!
8b954bc
to
64839a4
Compare
64839a4
to
ce27ec6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 🎉
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed \Throwable
support here, opened #155.
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 aPSR-7 Response
. If the response need time be created, a promise must be used (checkoutexamples/04-stream-request.php
).The new
Response
extends fromRingCentral\Psr7\Response
but adds the possibility to add an ReadstreamInterface implemnation as body.Refs #28
Builds on top of #146