Support parsing urlencoded request body (RequestBodyParserMiddleware) #220

WyriHaximus · 2017-09-12T20:46:37Z

Body parser for #215
To be used with #216

Todo:

Middleware
Basic Tests
Readme

jsor · 2017-09-14T13:56:36Z

src/Middleware/RequestBodyParserMiddleware.php

+    /**
+     * @param bool $keepOriginalBody Keep the original body after parsing or not
+     */
+    public function __construct($keepOriginalBody = false)


Shouldn't this be true by default to replicate PHP's default behaviour (and that of most PSR-7 implementations)?

I agree 👍 I'd also vote to completely remove this parameter for now as the motivation is a bit unclear, plus we can still add this in a follow-up PR.

jsor · 2017-09-14T14:04:02Z

I'd prefer a dedicated UrlEncodedRequestBodyParserMiddleware without the addType() extension point. Smells like some kind of middleware-in-middleware system ;-) WDYT?

christoph-kluge · 2017-09-14T14:23:30Z

I'd prefer a dedicated UrlEncodedRequestBodyParserMiddleware

I'd also prefer a dedicated middleware per type but on the other
hand I also like $parser->addType('application/json', new JsonRequestBodyParser()).

christoph-kluge · 2017-09-14T14:45:20Z

src/Middleware/RequestBodyParserMiddleware.php

+        }
+
+        try {
+            $value = $this->types[$type];


$value might be a bit unclear here. Maybe something more readable like $request = $parser($request) ?

christoph-kluge · 2017-09-14T14:45:59Z

src/Middleware/RequestBodyParserMiddleware.php

+            /** @var ServerRequestInterface $request */
+            $request = $value($request);
+        } catch (\Exception $e) {
+            return $next($request);


Should we log somehow the parsing failure?

We could allow a PSR-3 logger as constructor argument and log errors to it, or emit errors as Server does

It'd be interesting what PHP's behaviour is in case of invalid request bodies. I think we should probably follow that behaviour.

WyriHaximus · 2017-09-14T16:10:50Z

@jsor @christoph-kluge The thing is this set up allows us to add more types in the future, infect I already have a multipart PR ready for when this PR is merged:

final class RequestBodyParserMiddleware
{
    private $types = array();

    public function __construct()
    {
        $this->addType('application/x-www-form-urlencoded', function (ServerRequestInterface $request) {
            $ret = array();
            parse_str((string)$request->getBody(), $ret);

            return $request->withParsedBody($ret);
        });
        $this->addType('multipart/form-data', function (ServerRequestInterface $request) {
            return MultipartParser::parseRequest($request);
        });
        $this->addType('multipart/mixed', function (ServerRequestInterface $request) {
            return MultipartParser::parseRequest($request);
        });
    }
}

jsor · 2017-09-14T17:53:59Z

@WyriHaximus That would be the MultipartRequestBodyParserMiddleware. Your solution introduces yet another configuration layer.

WyriHaximus · 2017-09-14T18:24:19Z

@jsor With solid defaults, which can be extended. But I get your point will file a PRs for both MultipartRequestBodyParserMiddleware and UrlEncodedRequestBodyParserMiddleware middleware so we can compare 👍

WyriHaximus · 2017-09-14T21:33:14Z

@jsor Filed them as #222 and #223

WyriHaximus · 2017-09-17T13:12:57Z

Also closed #200 and #202 since #222 and #223 are superseding those as middlewares (thanks for the nudge @andig).

WyriHaximus · 2017-09-19T16:10:05Z

After discussing this PR with @clue and @jsor we decided to go with a slight alteration in this middleware. or view command line instructions: removing the ability to add additional parsers for custom types and only to include parsers for body types from the HTTP 1.0/1.1/2.0 specs.

I'll add documentation later today

WyriHaximus · 2017-09-19T21:59:31Z

Documentation added, @jsor @clue could you review?

jsor · 2017-09-20T06:04:29Z

README.md

+
+```php
+$middlewares = new MiddlewareRunner([
+    new RequestBodyParserMiddleware(),


Should probably have added the RequestBodyBufferMiddleware before to document the requirement.

jsor · 2017-09-20T06:18:18Z

src/Middleware/RequestBodyParserMiddleware.php

+    {
+        $type = $request->getHeaderLine('Content-Type');
+
+        if ($type === 'application/x-www-form-urlencoded') {


I'm trying to find out since ever if header field values are case-sensitive. I'm talking only about the values, as header names are clearly defined case-insensitive. I've only found some definitions for specific header values, eg. for Connect or Transfer Coding names.

Not sure, but it probably won't hurt to lowercase $type here.

Not sure either, but I've updated the middleware forcing it to lowercase 👍

For the reference: This change is correct 👍 Header field values are not case-insensitive per se, but only for certain header field names, including this one:

The type, subtype, and parameter name tokens are case-insensitive.

https://tools.ietf.org/html/rfc7231#section-3.1.1.1 and https://tools.ietf.org/html/rfc1866#section-8.2.1

jsor · 2017-09-20T06:22:02Z

README.md

@@ -725,6 +726,38 @@ $middlewares = new MiddlewareRunner([
 ]);
 ```

+#### RequestBodyParserMiddleware
+
+Another built-in middleware is `RequestBodyParserMiddleware` which takes a fully buffered request body 


I think this should read just

The RequestBodyParserMiddleware takes a fully buffered request body...

Referring to previous paragraphs and repeating "built-in middleware..." might be become tedious once we add more and more middleware.

…comment)

andig · 2017-09-20T12:17:40Z

@WyriHaximus as successor of #223 this PR is now missing the multipart parts? Will multipart be another middleware or integrated here?

WyriHaximus · 2017-09-20T12:23:18Z

@andig multipart will be filed in a follow up PR once this one is in.

…$parser

clue · 2017-09-25T18:40:40Z

I like the direction this PR is heading and agree that it makes sense to keep "default body parsers" ("what PHP does") in a single middleware because it's unlikely a consumer would want to access individual parsers anyway. As described by @WyriHaximus, multipart handling will be added to the same code base in a separate PR.

I'm currently in the process of rebasing and squashing some of these changes and will update this PR with some added tests. Other than that, this LGTM

geggleto · 2017-09-25T18:42:46Z

src/Middleware/RequestBodyParserMiddleware.php

+    {
+        $type = strtolower($request->getHeaderLine('Content-Type'));
+
+        if ($type === 'application/x-www-form-urlencoded') {


Feel free to copy from here :) https://github.com/slimphp/Slim/blob/3.x/Slim/Http/Request.php#L204-L241

Great work btw :)

See #225 :-)

https://github.com/WyriHaximus/reactphp-http-middleware-custom-request-body-parsers 😎

WyriHaximus · 2017-09-25T18:44:30Z

@clue awesome 🎉

reactphp#220 (comment)

clue

Nice! 🎉 I've rebased your changes and squashed a number of related commits after adding some tests to ensure this has 100% coverage, now let's get this in!

geggleto · 2017-09-25T18:52:25Z

src/Middleware/RequestBodyParserMiddleware.php

+
+use Psr\Http\Message\ServerRequestInterface;
+
+final class RequestBodyParserMiddleware


I would like to not see final in this case. There are literally a metric ton of different request body formats and this is rather limiting (from the perspective of a http framework). XML, Json, form-urlencoded are only the most popular however there are many custom encodings as well.
For framework usage it would be nice to be able extend.

Also it's possible to have a mixture of encoding types inside an application (I know it sounds weird, but it happens... a lot).

@geggleto going to provide a third party middleware that allows this as I also have a use for it 😄

See #225 :-)

@geggleto We've decided to provide only some core middleware which are required to build a standard PHP/PSR-7 request. Everything else can be easily provided third-party middleware packages. See #221 for reference.

https://github.com/WyriHaximus/reactphp-http-middleware-custom-request-body-parsers 😎

WyriHaximus requested review from jsor and clue September 12, 2017 20:46

WyriHaximus added easy pick help wanted new feature labels Sep 12, 2017

WyriHaximus added this to the v0.8.0 milestone Sep 12, 2017

jsor reviewed Sep 14, 2017

View reviewed changes

christoph-kluge reviewed Sep 14, 2017

View reviewed changes

This was referenced Sep 14, 2017

Form Urlencoded Request Body Parser Middleware #222

Closed

Multipart Request Body Parser Middleware #223

Closed

This was referenced Sep 17, 2017

Streaming body parser: Multipart #200

Closed

Streaming body parser: Form Urlencoded #202

Closed

jsor reviewed Sep 20, 2017

View reviewed changes

WyriHaximus added a commit to WyriHaximus/http that referenced this pull request Sep 20, 2017

Force Content-Type header contents to lowercase as per reactphp#220 (…

91d8d5c

…comment)

jsor approved these changes Sep 20, 2017

View reviewed changes

WyriHaximus added 2 commits September 25, 2017 20:27

RequestBodyParser middleware

a4e9092

Removed $keepOriginalBody constructor argument and renamed $value to …

485793c

…$parser

geggleto reviewed Sep 25, 2017

View reviewed changes

WyriHaximus added 2 commits September 25, 2017 20:45

Removed the ability to add custom parsers for custom body types

3c94b05

Force Content-Type header contents to lowercase as per

a0c8de7

reactphp#220 (comment)

clue force-pushed the middleware-body-parser branch from 91d8d5c to a0c8de7 Compare September 25, 2017 18:49

clue approved these changes Sep 25, 2017

View reviewed changes

geggleto reviewed Sep 25, 2017

View reviewed changes

WyriHaximus merged commit cb4cf5e into reactphp:master Sep 25, 2017

clue changed the title ~~Middleware request body parser~~ Support parsing request body (RequestBodyParserMiddleware) Sep 25, 2017

clue removed easy pick help wanted labels Sep 25, 2017

geggleto mentioned this pull request Sep 25, 2017

Additional Body parsers #225

Closed

clue changed the title ~~Support parsing request body (RequestBodyParserMiddleware)~~ Support parsing urlencoded request body (RequestBodyParserMiddleware) Sep 25, 2017

WyriHaximus mentioned this pull request Sep 25, 2017

Support multipart parsing for RequestBodyParserMiddleware (file uploads) #226

Merged

2 tasks

WyriHaximus added a commit to WyriHaximus/reactphp-http-middleware-custom-request-body-parsers that referenced this pull request Sep 25, 2017

Custom request body parsers, spunoff from reactphp/http#220 (comment)

d54d33f

andig mentioned this pull request Oct 2, 2017

[WIP] Update React components, mainly HTTP php-pm/php-pm#226

Closed

10 tasks

This was referenced Nov 17, 2017

Respect ini settings for file uploads #250

Closed

Add file upload example and documentation #252

Merged

RequestBodyParserMiddleware should respect ini settings #257

Closed

clue mentioned this pull request Apr 28, 2023

HTTP client: Simplify HTTP file uploads and submitting forms #496

Open


		use Psr\Http\Message\ServerRequestInterface;

		final class RequestBodyParserMiddleware

Support parsing urlencoded request body (RequestBodyParserMiddleware) #220

Support parsing urlencoded request body (RequestBodyParserMiddleware) #220

Conversation

WyriHaximus commented Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsor commented Sep 14, 2017

christoph-kluge commented Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Sep 14, 2017

jsor commented Sep 14, 2017

WyriHaximus commented Sep 14, 2017

WyriHaximus commented Sep 14, 2017

WyriHaximus commented Sep 17, 2017

WyriHaximus commented Sep 19, 2017

WyriHaximus commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andig commented Sep 20, 2017

WyriHaximus commented Sep 20, 2017

clue commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Sep 25, 2017

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Sep 12, 2017 •

edited

Loading

christoph-kluge commented Sep 14, 2017 •

edited

Loading