Skip to content
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

Use PSR-7 RequestInterface instead of internal Request object #146

Merged
merged 2 commits into from
Mar 24, 2017

Conversation

legionth
Copy link
Contributor

@legionth legionth commented Mar 16, 2017

This PR uses the PSR-7 RequestInterface to replace the given Request class.

A HttpBodyStream will be used as body for the RequestInterface, 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 getQueryParams will be removed with the Request class. This will be handled in a seperated PR which will implement the ServerRequestInterface

Refs #28
Builds on top of #97

@legionth legionth changed the title Use PSR-7 RequestInterface instead of internal Request object Use PSR-7 ServerRequestInterface instead of internal Request object Mar 16, 2017
@clue clue added this to the v0.7.0 milestone Mar 16, 2017
@legionth legionth force-pushed the handle-psr7-requests branch 10 times, most recently from 1076a45 to ca091f8 Compare March 17, 2017 10:48
@legionth legionth force-pushed the handle-psr7-requests branch from ca091f8 to 4a0b4d7 Compare March 17, 2017 10:49
@legionth legionth changed the title Use PSR-7 ServerRequestInterface instead of internal Request object Use PSR-7 RequestInterface instead of internal Request object Mar 17, 2017
@legionth
Copy link
Contributor Author

Ping @clue. Let me know what you think about the documentation.

Copy link
Member

@clue clue left a 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");
}
Copy link
Member

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
These methods SHOULD NOT be called, because these are not compatible
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:
Copy link
Member

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
$response->writeHead(400, array('Content-Type' => 'text/plain'));
$response->end("An error occured while reading at length: " . $contentLength);
});
});
```

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.
Copy link
Member

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
An error will just `pause` the connection instead of closing it. A response message
can still be sent.

A `close` event will be emitted after an `error` or `end` event.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@legionth legionth force-pushed the handle-psr7-requests branch 3 times, most recently from 524fe9d to 40cac8b Compare March 22, 2017 10:45
Copy link

@User3061 User3061 left a 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.

@legionth legionth force-pushed the handle-psr7-requests branch from 40cac8b to 7ec68c5 Compare March 23, 2017 13:01
@legionth
Copy link
Contributor Author

legionth commented Mar 23, 2017

Removed the latest commits about the documentation. @clue wanted to add the documentation for this PR.

@clue clue force-pushed the handle-psr7-requests branch 2 times, most recently from 4cf0b3e to 7891316 Compare March 23, 2017 20:46
@clue clue force-pushed the handle-psr7-requests branch from 7891316 to a585e5d Compare March 23, 2017 20:48
@clue
Copy link
Member

clue commented Mar 23, 2017

Updated to include documentation :shipit:

After talking to @legionth about the documentation, I've offered to support bringing this in line with the stream documentation 👍

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yey :shipit:

At first sight, this looks like a pretty massive PR, but the changes in the examples highlight how little this affects customer code 👍

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@WyriHaximus WyriHaximus merged commit 283e842 into reactphp:master Mar 24, 2017
@andig
Copy link
Contributor

andig commented Mar 24, 2017

The inline docs of Server still contain the old request object: https://github.com/reactphp/http/blob/master/src/Server.php#L95

@clue
Copy link
Member

clue commented Mar 24, 2017

@andig Thanks for spotting, I'll take care of this in a follow-up PR! 👍

@stefanotorresi
Copy link

stefanotorresi commented Mar 31, 2017

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 React\Stream\ReadableStreamInterface".

Is there no other way?

@clue
Copy link
Member

clue commented Mar 31, 2017

but you can't actually use that interface

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? 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants