InputBag::get() should be smarter

[vhenzl]

While technically correct, the change in 716a70d leads to false positive reports for code like this:

$abc = $request->query->get('abc');
Assert::notNull($abc);
expectString($abc); // 💥 Parameter #1 $str of function expectString expects string, bool|float|int|string given.

AFAIK Symfony never converts query string values from string to other types. So unless the $defaultValue argument is specified, InputBag::get() always returns string|null. And if $defaultValue is specified, the return type should be string|T where T extends string|int|float|bool|null.

[ondrejmirtes]

Symfony is really confusing here. This change was triggered by symfony/symfony@381a0a1 - I guess we'd have to make InputBag generic and differentiate between POST and other uses... But I'm not fluent in Symfony enough to know all the places where InputBag can be retrieved from to mark them correctly.

[jaydiablo]

We recently upgraded from 0.12.27 to 0.12.38 of this plugin and started getting a lot of false positives on code like this:

$request->request->get('field_name', '');

$request->request is an InputBag, and the previous behavior seemed that it would use the second parameter of the get method to determine what the type would be (in this case, a string), now it appears that it sees it as bool|float|int|string.

[benr77]

I agree - it should look at the second parameter to get() which is the default value and determine the expected type from that. If it's not specified, it should fall back to string|null

[ondrejmirtes]

@benr77 I disagree. You can have $inputBag->get('foo', 1) and it can still return string.

[benr77]

@ondrejmirtes OK good point you are correct.

The problem remains though - how do we get around all these false positives? Is there a workaround or is just ignoring them for the moment the best solution? Thanks

[ondrejmirtes]

See #177 (comment) - if someones compiles a list of all possible places to get InputBag in a typical Symfony application, we can differentiate between POST and other usages.

[benr77]

Well InputBag is only used for POST/GET/COOKIE parameters, so certainly for any correctly designed app this should only be used in controller classes. I have checked several of my Symfony apps and the only place I see new InputBag() is in the http-foundation/Request.php

In the 5.1.0 changelog there is:

Deprecated retrieving non-string values using InputBag::get(), use InputBag::all() if you need access to the collection of values

Based on this, can we assume string|null is the expected return type of InputBag::get() ??

[ondrejmirtes]

This commit shows that the intended message is "non-scalar" values, not "non-string" values: symfony/symfony@381a0a1

I'm asking how a GET InputBag is retrieved as opposed to POST InputBag for example...

[benr77]

Do you mean:

/** @var Request $request */

// POST vars InputBag
$request->request

// GET vars InputBag
$request->query

//COOKIES vars InputBag
$request->cookies

https://symfony.com/doc/current/components/http_foundation.html#accessing-request-data

[ondrejmirtes]

Two more questions:

1. Any other way to obtain an InputBag?
2. Is it usual for users to typehint InputBag in their application somewhere?

[benr77]

1. Not to my knowledge - but I suppose you could leverage it directly yourself for some reason, but why this would be useful over just using ParameterBag directly I don't know.
2. No, I don't think this would be very likely.

[ondrejmirtes]

Alright, so my plan is to introduce some stubs that make InputBag generic, and then to typehint those Request properties properly.

[vhenzl]

Generic InputBag should be fine for $request->query and $request->cookies but I'm not sure about $request->request. The request field can hold decoded JSON payload. It's mentioned in symfony/symfony#40315 (comment) and is the reason why the type-hint in InputBag::get() has been updated in the first place.

Anyone could theoretically replace their query and cookies as well but I can't see any reason why anyone would do that.

It'd done by replacing the (until then empty) request by decoded content; basically $request->request->replace(json_decode($request->getContent(), true)).

Symfony doesn't have any built-in mechanism to do so, it's up to the developer to do it manually or use packages like https://github.com/symfony-bundles/json-request-bundle or https://github.com/qandidate-labs/symfony-json-request-transformer.

There is also Request::toArray() that has been introduced recently (Symfony 5.2) and is meant for obtaining parsed JSON, but I reckon that majority still uses request for it (which IMO is a more correct approach).

[ondrejmirtes]

@vhenzl So is this fine or incorrect? What would you do?

	/**
	 * Request body parameters ($_POST).
	 *
	 * @var InputBag<string|int|float|bool>
	 */
	public $request;

[ondrejmirtes]

Please test this before I release it: 95d9ae7 (you'll need phpstan/phpstan dev-master too)

[vhenzl]

So is this fine or incorrect?

Fine. It should be a sensible default for most people I believe.

I have just realised that InputBag:::get() can't return array (on purpose) so it makes it kinda unusable for parsed JSON. But if you have a parsed JSON payload in $request->request, you are unlikely to access it via get() anyway, you would just get the whole thing via $request->request->all().

So the use case for request->get() is probably only with form submissions. And in that case, the return type is likely to be string|null only, but having string|int|float|bool|null is safer.

Having string|null for query and cookies should be perfectly fine as their content comes directly from $_GET and $_COOKIE.

[vhenzl]

Thanks, @ondrejmirtes!

I tried it and it works as expected for me. Needless to say, though, that we have just a few places with get(), the majority of our code uses all(), so @jaydiablo and @benr77 are better to give it try as well.

[benr77]

I just tested it too and it seems to be working well. I would be happy for you to release. Thanks for your efforts!

[ondrejmirtes]

Awesome :) I'm gonna release it along with PHPStan 0.12.97 in the coming days.

[Tobion]

The solution does not seem right to me. See 95d9ae7#r56194881
I get alot of false positive phpstan errors like

Call to function is_string() with string will always evaluate to true.

[ondrejmirtes]

@Tobion When do you think it doesn't return a string?

[Tobion]

The setter accepts any scalar. So the getter can also return any scalar.

[ondrejmirtes]

@Tobion Do you have a use-case where you're setting values to an InputBag that you've got from Request through one of the public properties? Please open a new issue about that.

[Tobion]

I don't know. But if it doesn't make sense, it shouldn't be like this in Symfony. It was changed recently to better support static analysis, but apparently it didn't make it better then if there are still workarounds needed.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.