[HttpFoundation] Deprecation error null not allowed InputBag::set()

[krioo]

Symfony version(s) affected: 5.1.0

Description
Calling InputBag::set()with the second argument set to null triggers deprecation error:

Since symfony/http-foundation 5.1: Passing "null" as a 2nd Argument to "Symfony\Component\HttpFoundation\InputBag::set()" is deprecated, pass a string or an array instead.

This behaviour causes issues when replacing JSON request body with the JSON decoded value as JSON supports null values.

How to reproduce

/** @var Symfony\Component\HttpFoundation\Request $request */
$request->request->set('key', null);

Possible Solution
Allow null values by adding is_null(), since the currently used is_scalar(null) returns false: https://www.php.net/manual/en/function.is-scalar.php

[nicolas-grekas]

This looks a lot like #36725
Swapping the request arguments after decoding a json looks like a hack to me.
I would recommend putting the decoded JSON into a request attribute instead.
(which is what I wrote on the linked issue also.)

[chilimatic]

Is it a hack? I would rather see it as an IO-Result

Either the body is a form, in this cases you're reasoning is very valid. but modern infrastructure shifts toward json bodys or other content. which are not really "parameters" since they are content type json.

if the whole body is JSON and JSON has the allowed type of null it does make sense to allow null values.

Personally I would say that's a monomorphism/transforming or a strategy/adapter pattern -> the input which can be CT1 | CT2 -> IT (content-type1, content-type2 should become input-type (input interface)).

So as a developer I would expect it to work in a defined way. Esp since bundles implemented it in a similar way.

Maybe I am missing the real implications in the Framework that make it "bad" to use null values. Because if there are no directly affected parts one might argue it's primarily a question of taste.

I don't mean any disrespect just thinking out loud here.

[ro0NL]

What's wrong with something like

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

IMHO InputBag is designed to be bound to $_POST/$_GET.

[chilimatic]

I see the reasoning behind $_POST/$_GET.

I still can be hacky as long as I cast it to string? or any other scalar value which would be in this union type variance? and arrays?

The rest of possible assumptions aside. what's the purpose and how do we benefit from it not being null?

[chilimatic]

I still can be hacky as long as I cast it to string? or any other scalar value which would be in this union type variance? and arrays?

@ro0NL My answer referred to a comment in the reference ticket provided by @nicolas-grekas

and the question is still the same, what's the benefit because it's defined as

is_scalar|array|has->__toString() union at least according to the notice. So I can actually inject any object that implement that magic method, but not null? and this relates to $_GET/$_POST? but bool as a scalar is valid? which can only be sent as number represented as a string? I come from low-level languages maybe that's my problem.

you see my problem with the implementation? and why I mentioned the scalars as union types?

I know of PHP-Type coercion problems, so it's not I don't get why or how you got there.
I can guess you guys have other things to do, which I respect also I appreciate your work.

I just would like to understand the underlying reasoning and conclusions why. Currently I got

• it feels hacky (opinion)
• it represent $_GET, $_POST, $REQUEST, $COOKIE <- which leads to __toString, bool, int but not null
• you can use this way to do things (which I highly appreciate but it does not answer the question)

the programmer in me accepts the "you can do it that way" the CS-person is basically confused by the variance/implementation.

[ro0NL]

i agree there are few unclarities :) see also #34363 (comment)

I think ultimately it'll rely on implicit string casting when accessed: https://3v4l.org/IIWBt

But then we should actually cast it today manually for consistent behavior (e.g. the same behavior with or without a real return type).

The array case should probably validated/documented to be string[] (any no. of levels actually, but i dont think there's a notation for that).

InputBag is meant to make accessing single/multi values explict (?k=v vs. ?k[]=v), as the user is free to mangle those, throwing "400 bad request" as such. That's the USP of InputBag at least.

I tend to agree the same case may apply to JSON decoded payloads.

So yes, given scalar is already allowed (not strictly string) it probably makes sense to allow null as well, esp. since it's already anticipated in the return type.

[nicolas-grekas]

The request's body is not always populated via $_POST.
Can't we use InputBag only when $_POST is used?
That would fix the issue, isn't it?

Anyone up for a PR?

[krixon]

I don't think that addresses the original issue. It's a pretty common pattern in projects I've worked on to replace the request parameters with decoded JSON which might contain null values. I know a request attribute could be used here instead, but that means everything has to know to examine that attribute instead of using Request::request which is much more natural and obvious IMO.

[nicolas-grekas]

I don't think that addresses the original issue.

Can you explain why? Isn't my proposal compatible with extracting request values from json bodies?

[krixon]

Generally, I will have an event listener which looks something like this:

public function onKernelController(ControllerEvent $event) : void
{
    $request = $event->getRequest();

    if ($request->getContentType() !== 'json' || empty($request->getContent())) {
        return;
    }

    try {
        $payload = json_decode($request->getContent(), true, 512, JSON_THROW_ON_ERROR);
    } catch (JsonException $exception) {
        throw new BadRequestHttpException('Invalid JSON body.', $exception);
    }

    $request->request->replace(is_array($payload) ? $payload : []);

Assuming the request method causes $_POST to be populated, which is the case for all of the common unsafe HTTP methods an API is likely to encounter, this code breaks (edit: not breaks, but triggers a deprecation notice) with 5.1 but works fine with <= 5.0. I'm not sure how the restricting the use of InputBag to $_POST data addresses this problem (apologies if I have misunderstood your proposal).

BTW, I have worked around this for now by simply replacing the entire InputBag:

$request->request = new ParameterBag(is_array($payload) ? $payload : []);

As a side note, the change has also caused some issues elsewhere in my APIs - for example I will often accept a list of sort keys in the query string for lists resources, and for client convenience I will accept a single string or an array of strings. This seems to be pretty much incompatible with the new InputBag API - I am forced to pull out all of the parameters in this situation and extract the value from that:

// Previously:  $request->query->get('sort');
$status = $request->query->all()['sort'] ?? null;

Not a huge deal, but seems a little more clumsy to use.

[nicolas-grekas]

Assuming the request method causes $_POST to be populated

PHP also checks the content-type before populating $_POST.
This means your case is safe with my approach.

I am forced to pull out all of the parameters in this situation and extract the value from that:

yes, that's on purpose: most ppl don't use query parameters in the way you describe - the defaults are to make them safer. Now, it also means some use cases will need to adapt. You just described one :)

[krixon]

PHP also checks the content-type before populating $_POST.

Ah of course, $_POST is only populated for application/x-www-form-urlencoded and multipart/form-data content types. That makes sense then and I agree this would resolve the issue.

Is this just a case of changing:

$this->request = new InputBag($request);

to:

$this->request = empty($_POST) ? new ParameterBag($request) : new InputBag($request);

in Request::initialize()? Feels a little weird given this method otherwise isn't aware of globals though.

[ro0NL]

im still generally curious why InputBag wouldnt fit JSON payloads :/

e.g. if a user is posting {"key": "val"} instead of {"key": ["val"]}, then InputBag::get/all('key') is generally helpful for throwing 400 bad request no?

Can't we use InputBag only when $_POST is used?

it's also used for $_GET/$_COOKIE (edit: oh that's per property decided ofc.). I agree setting null values manually makes less sense, but we can argue about the mutability of InputBag in the first place.

[nicolas-grekas]

The request object is mutable. Immutability is provided by cloning, that's the design.
About linking $_POST and InputBag, I only meant this for the "request" property. Cookie/query should remain as is.

[nicolas-grekas]

See #37265

[gmponos]

Still I am against the InputBag. Not sure how much constructive I can be here and how I can elaborate that DX is thrown out of the window with the InputBag. Your changes in the related PR make the behavior in my PoV even further inconsistent. One time the request behaves X and the other behaves Y, right?

For instance, you might have listeners that are running after authentication, that are agnostic of the content-type of the request and ppl will be driven to use always the all function.

I changed my Authenticator from :

        $login = $request->request->get('login');
        return \is_array($login) && isset($login['username']) && isset($login['password']) && isset($login['_token']);

to:

        $request = $request->request->all();
        if (!isset($request['login'])) {
            return false;
        }

        $login = $request['login'];
        return \is_array($login) && isset($login['username']) && isset($login['password']) && isset($login['_token']);

Trivial changes you might say.. but I don't find the benefit.. I still need to check the login if it is actually an array... The notion is that ppl were getting 500.. those ppl should just fix their code. I was aware and happy with what request of sf was offering.. A pile of mix data with No Behavior when I fetch them.. and I was fine with that...

Some more:

Not a huge deal, but seems a little more clumsy to use.

From here

To get the raw value, thus string|array use all()['key'].

From my issue before.. The solution suggested there above kills the api of get as I already said...