[HttpFoundation] Allow Request#request to be ParameterBag

[wouterj]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

The InputBag story starts to walk circles, but I hope this is the last PR in the story.

1. It's very common in Symfony applications to put the decoded JSON request body in $request->request. [HttpFoundation] Add InputBag #34363 broke this use-case
2. In [HttpFoundation] Deprecation error null not allowed InputBag::set() #37100 (comment), it was decided that $request->request can be allowed to be InputBag|ParameterBag (by only using InputBag for form requests). Introduced by [HttpFoundation] use InputBag for Request::$request only if data is coming from a form #37265
3. Later, this was reverted by [HttpFoundation] Use InputBag for POST requests too #40315 as another fix for InputBag was merged. While this fixes null and other scalar types, it doesn't fix the array type (which is perfectly fine if you're doing $request->request = new ParameterBag(json_decode(...));)

I think, to support putting JSON decode in $request->request, we must make it a union of ParameterBag|InputBag.

I'm not sure about the branch to target branch, feel free to change

[nicolas-grekas]

I'm not really convinced we need this. What's the real motivation here?
PPl should know when they expect an array or a scalar, so InputBag is fine to me, and it allows more accurate behaviors for error reporting.

[wouterj]

The main thing is when doing something like:

$request->request = new ParameterBag(json_decode($request->getContent()));

This has been the standard in FOSRestBundle since the start, making it a common practice in Symfony API implementations now (or let's talk only about what I know: we're still using this in our controllers as a leftover legacy from the original FOSRestBundle implementation we used to use).

It's not so nice to use InputBag here, as nested arrays are pretty normal in JSON request bodies. This is not a problem currently, as the property is not typed yet. But it will become a problem in Symfony 7.

Thinking about it, I think there are 2 possibilities:

1. This PR, but I agree that a union-typed public property doesn't really help anyone
2. Adding InputBag#getArray(). This means the bag has 2 possibilities: either get a scalar (get()) or an array (getArray()). This seems, with PHP's type system, a pretty solidly typed bag (contrary to a previously proposed getAny(), which would be too loose).

What do you think about (2)? (for 6.1 of course)

[ro0NL]

3. symfony/src/Symfony/Component/HttpFoundation/Request.php

   Line 1479 in 0d6e859

   public function toArray(): array

i think semantically $request->request may fit any decoded payload. But sticking with form-urlencode, thus InputBag, now works for me.

we could explore eg. dot-notation / property-access for InputBag

[wouterj]

Oh, didn't know about toArray(). That seems useful (it may make sense to add some caching, to avoid decoding every time we call it for the same request?)

[ro0NL]

🤷 aiming for uniform acces thru either InputBag or toArray sounds noble.

->toArray = json
->request = form-urlencode aka $_POST

is maybe weird

[nicolas-grekas]

InputBag|ParameterBag is blurring the line. toArray()has been added exactly for that use case (json) (I also forgot about it :X)
Closing therefore.

[ro0NL]

Note $r->request = new InputBag($r->toArray() should generally work, to deal with 400 bad request.

SF just cant do it in a lazy manner (that is decoding when calling InputBag::get). I'd still think that's a noble goal, if even behind a framework config flag.

[ddr5292]

You folks need to now explain to all your users how to fix this problem your comments here thus far have been unclear. Consider that the way that in the npm/composer worlds, we now have third party code that is failing all over the place (and it is 16 months on.) That was a reckless change!

[ro0NL]

@ddr5292 please open an issue if you believe anything's wrong still.

[ddr5292]

Please document your changes and explain how to change our code so that we can use get past this error now. Give multiple examples for all use cases.

[derrabus]

@ddr5292 Please open an issue and explain your problem instead of ranting in the comments of PRs that have been closed for years.

[Nerogee]

@ddr5292 Please open an issue and explain your problem instead of ranting in the comments of PRs that have been closed for years.

If you have a really big project using get() everywhere, then you know what problem it is. I'm facing the same issue after trying the upgrade and now started to doubt if it's the right thing to upgrade. we used get() for either string or array (json). And then we have logic in our code to make sure submitted value is in desired data type. (and I think it's programmers' responsibilities to validate data type before using instead of framework itself). The inputBag is doing too much job.

With this breaking change, I need to review all get() and think about if that submitted value is supposed to be scalar or array. This process is tedious and error prone.

I personally agree typed method is good for more predictable behavior. But the question is does it really worth introducing breaking change? why not keep get() as what it was and add getArray() for those who need typed data ?

[wouterj]

This ship sailed long ago. As you can see, this has been discussed numerous times 2 years ago, so it's unlikely that the outcome changes now. Furthermore, both deprecation and new behavior has been part of Symfony for nearly 4 years. Changing or reverting now is impossible and will impact more applications than changing this in existing applications that are upgrading to 5.x or 6.x now.

Please also do not comment more or less the same things across multiple issues/PRs within the same topic. This divides the same discussion across multiple places, making it harder to track.

[derrabus]

I'm locking all of those issues now. If there's something NEW to discuss, please open a new issue.