Parsed form urlencoded requests.

[nesl247]

I'm not sure why urlencoded requests weren't being parsed.

[zf2timo]

Why was this test case Changed?

[nesl247]

Because this makes POST of application/x-www-form-urlencoded an applicable type, thus the test would break if I didn't change it.

[weierophinney]

They don't need to be parsed in most cases.

Diactoros uses the value of $_POST to seed the body parameters by default when creating the initial server request, and the other psr-7 implementations I've seen in the wild do so as well. Adding that here duplicates the parsing, adding overhead.

[nesl247]

Are you saying $request->getBody()->getContents() is supposed to be returning an array (basically the value of $_POST)? Because that's not what I'm experiencing with zend-expressive at all.

Also, if they are doing that, it seems very wrong as getBody() should be the raw, unmodified contents shouldn't it? Otherwise what is the purpose of getParsedBody()?

[weierophinney]

Are you saying $request->getBody()->getContents() is supposed to be returning an array

No, I'm saying $request->getParsedBody() should be returning the value of $_POST by default, if that value was populated at the time of the initial request. And that's the reason the FormUrlEncodedStrategy was not doing any parsing by default for POST requests (it's only needed for PUT and PATCH).

[nesl247]

I was not experiencing what you are saying. It wasn't until I made these changes that I got the values of $_POST. I used getParsedBody() before I even looked at the strategy thinking it was already doing this, and then when I saw it wasn't, I started working on this PR.

[weierophinney]

I'd argue that if we do this, the following should occur:

$parsed = $request->getParsedBody();

if (! empty($parsed)) {
    return $request;
}

$raw = $request->getBody()->getContents();
if (empty($raw)) {
    return $request;
}

parse_str($raw, $parsed);
return $request->withBodyParams($parsed);

This would ensure that parsing only happens (a) if the parsed body is empty AND (b) if we have raw body contents.

This would require:

• Reverting the data provider change you made (it will still pass with the above changes)
• Adding new test cases for the new behavior

UPDATED to separate the parse_str call from the withBodyParams() call.

[weierophinney]

I was not experiencing what you are saying.

I've used this myself on several sites, and have it working exactly as I described. The only thing I can think of is that it may be a difference in web server or how you're creating the ServerRequest instance, as the ServerRequestFactory::fromGlobals() method populates the parsed body from $_POST by default.

Anyways, with the changes I suggest above, we would eliminate unnecessary double-parsing, while providing a way to parse if it's not populated.

[nesl247]

Ok, I'll work on the changes based on what you said. And your explanation is definitely the reason. It's actually a functional test client I created for an application where I'm having the issue. We build up the request and pass it to the application to run it as it makes it much quicker and easier than going against a real server.

With these changes, at least we'll be guaranteed that this middleware will work regardless of how the request was built, which I think is the right way to do it.

I'll get to work on these changes sometime this week then as I have to switch focus to another project for a bit.

Thanks!

[michaelmoussa]

@nesl247 I've flagged this for 2.1.0 in hopes that I can wrap up the remainder of pending issues and PRs in that release. Please let me know if you need some help with this, or if you think you'll be able to make the discussed changes yourself before long.

[michaelmoussa]

Looks good to me. Will be released with 2.1.0.