-
-
Notifications
You must be signed in to change notification settings - Fork 167
Use PSR-7 RequestInterface instead of internal Request object #146
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
Conversation
1076a45 to
ca091f8
Compare
ca091f8 to
4a0b4d7
Compare
|
Ping @clue. Let me know what you think about the documentation. |
clue
left a 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.
Awesome feature, but this desperately needs some better documentation! 👍
README.md
Outdated
| $response->write("Request method: " . $request->getMethod() . "\n"); | ||
| if ($request->getBody()->getSize() !== null) { | ||
| $response->write("Request body size: " . $request->getBody()->getSize() . "\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.
This condition is a bit unclear, see also below.
README.md
Outdated
| with the `ReactPHP ReadableStreamInterface`. | ||
|
|
||
| Listen on the `data` event and the `end` event of the body of the [Request](#request) | ||
| to evaluate the data of the request 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.
This whole part appears really unclear. How about this:
As defined in PSR-7, the `getBody()` method returns a stream instance which implements the [PSR-7 StreamInterface](http://www.php-fig.org/psr/psr-7/#psrhttpmessagestreaminterface).
Note that the incoming request will be processed once the request headers have been received, which implies that the (potentially much larger) request body may still be outstanding (in a streaming state).
However, most of the `PSR-7 StreamInterface` methods have been designed under the assumption of being in control of the request body.
Given that this does not apply to this server, the following `PSR-7 StreamInterface` methods are not used and SHOULD NOT be called:
`tell()`, `eof()`, `seek()`, `rewind()`, `write()` and `read()`.
Instead, the returned stream instance *also* implements the [ReactPHP ReadableStreamInterface](https://github.com/reactphp/stream#readablestreaminterface) which gives you access to the incoming request body as the individual chunks arrive:
README.md
Outdated
| ``` | ||
|
|
||
| If the request body is chunked-encoded, the data will be decoded and emitted on the data event. | ||
| The `Transfer-Encoding` header will be removed. |
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 appears a bit unclear, see also above.
Also note its position, how about referring to the above example first and then proceeding with these internals? (telling a story)
How about this:
The above example simply counts the number of bytes received in the request body.
This can be used as a skeleton for buffering or processing the request body.
If you only want to know the request body size, you can use its `getSize()` method.
This method returns the complete size of the request body as defined by the message boundaries.
This value may be `0` if the request message does not contain a request body (such as a simple `GET` request).
Note that this value may be `null` if the request body size is unknown in advance because the request message uses chunked transfer encoding.
[simple example from above here?]
The server automatically takes care of decoding chunked transfer encoding and will only emit the actual payload as data.
The `Transfer-Encoding` header will be removed.
README.md
Outdated
| The constructor is internal, you SHOULD NOT call this yourself. | ||
| The `Server` is responsible for emitting `Request` and `Response` objects. | ||
|
|
||
| See the above usage example and the class outline for details. |
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.
?
524fe9d to
40cac8b
Compare
User3061
left a 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.
Good structure: short and clear.
40cac8b to
7ec68c5
Compare
|
Removed the latest commits about the documentation. @clue wanted to add the documentation for this PR. |
4cf0b3e to
7891316
Compare
7891316 to
a585e5d
Compare
|
Updated to include documentation After talking to @legionth about the documentation, I've offered to support bringing this in line with the stream documentation 👍 |
clue
left a 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.
Yey ![]()
At first sight, this looks like a pretty massive PR, but the changes in the examples highlight how little this affects customer code 👍
WyriHaximus
left a 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 inline docs of Server still contain the old request object: https://github.com/reactphp/http/blob/master/src/Server.php#L95 |
|
@andig Thanks for spotting, I'll take care of this in a follow-up PR! 👍 |
|
I know this has already been merged, but the results of this PR is basically "we implement PSR-7, but you can't actually use that interface, instead you should use Is there no other way? |
While I can't really support this statement, I do understand where you're coming from. Is there anything you're missing or suggesting in particular? 👍 |
This PR uses the PSR-7 RequestInterface to replace the given
Requestclass.A
HttpBodyStreamwill be used as body for theRequestInterface, which is a ReadableStreamInterface and a PSR-7 StreamInterface. So the body data can still be emitted asnychron and event-driven like before.Note: The method
getQueryParamswill be removed with theRequestclass. This will be handled in a seperated PR which will implement the ServerRequestInterfaceRefs #28
Builds on top of #97