Please rebase to 6.1. 5.4 is closed for features.

Please rebase to 6.1. 5.4 is closed for features.

done

This method might look trivial, but having a method that guarantees to return strings will improve the DX in a Symfony project with static analyzers a lot.

yes thanks, that's why I created this PR!

i'd prefer casting in user-land

Reminds me #39110
Do your mind change on my related PR @derrabus ? Thank you

This will trigger a PHP warning (notice in 7.4) and return "Array" string if parameter is an array. I understand developers should use this method only on validated requests, right? Otherwise an application will process this string as valid even though a user sent invalid data.

However this class has other shortcut methods which do not concern this case (e.g. getInt()), so...

This will trigger a PHP warning (notice in 7.4) and return "Array" string if parameter is an array. I understand developers should use this method only on validated requests, right? Otherwise an application will process this string as valid even though a user sent invalid data.

However this class has other shortcut methods which do not concern this case (e.g. getInt()), so...

The getString method is used when a string is expected. The getInt method will also contain incorrect array conversion logic and this warning will additionally inform the developer that he is using the ParameterBag incorrectly. In addition, if we talk about HttpFoundation, then we will never receive an array in the request.

Reminds me #39110 Do your mind change on my related PR @derrabus ? Thank you

May be you need use ParameterBag for options and arguments in Symfony\Component\Console\Input\Input::class?

In addition, if we talk about HttpFoundation, then we will never receive an array in the request.

Actually we will. This is the case when sending a field multiple times, like field[]=value&field[]=otherValue

Reminds me #39110 Do your mind change on my related PR @derrabus ? Thank you

I haven't actually. I realize that the situation appears similar to you, but let's keep the discussion about the Console componet in its dedicated tickets.

In addition, if we talk about HttpFoundation, then we will never receive an array in the request.

Actually we will. This is the case when sending a field multiple times, like field[]=value&field[]=otherValue

oh, yes

we could deprecate getInt() still :}

maybe put the methods on InputBag, where it's defacto safe to cast.

We currently have a discrepancy as getAlnum, getDigits, and getAlpha removes the non-valid characters, whereas getInt converts the value to an integer.

I would love to fix this inconsistency before adding getString.

I suggest that instead of letting PHP fail when a non-valid value is passed, like an array, we return an empty string for getString and probably 0 for getInt. I would also suggest to name it differently (like convertString?) to differentiate the two behaviors. That would mean renaming getInt as well (with a BC layer).

WDYT?

Nice suggestion, I added changes, please see my solution.

I think ParameterBag's helper methods like getInt, getString, getBoolean should not modify the returned parameter value but only change the type. If the type cannot be changed without changing the value then it should throw. In other words, a value '1' can be returned from getInt() as 1, but an array should not be converted to int. Similarly the floats: '1.1' != 1, I have put a parameter xyz with a value '1.1' to the bag and when I retrieve this parameter I receive other value. Let's think about this in terms of loose comparison: if $beforeValue != $afterValue then it should not be a valid casting.
In case of arrays a valid conversion would be ['1', '2', '3'] to [1, 2, 3] but not to int/string or if it removes any element.

PS. changing method names to "convert" prefix would be a little bit helpful but imo very dangerous because the consequences might be missed very easily. If you go this way I would add a warning in the doc blocks that this methods are not only for changing types but also might change the value.
PS2. I've just noticed that getBoolean might return a falsy false, because filter_var returns false on error.

@fabpot WDYT?

I just squashed+rebased the PR to target 6.2, please fetch and reset --hard before working again on the PR if you're still up to it. Please always rebase (never merge) when syncing with other branches.

If we go this way, I think we need to address the previous comments. Eg when an array is encountered, we should behave as InputBag does (aka throw). About the name of the new methods, what about asInt or toInt? Note that I would also suggest to rename all methods, eg getBoolean, etc.

About the name of the new methods, what about asInt or toInt? Note that I would also suggest to rename all methods, eg getBoolean, etc.

I would not invent a new name. getInt is consistent with the main method get and we already used that convention.

What's the status of this PR @nicolas-grekas ?
I'm really interested by this PR so I can finish the work if needed.

We have open questions on the new method names and error handling.
The method getInt having an inconsistent and problematic behavior, we cannot change this behavior while preserving backward compatibility.

InputBag throws BadRequestException in case of invalid data, while ParameterBag relies on TypeError.

Proposition:

• Deprecate getInt
• Add getInteger (rename convertInt in this PR). It is stricter on input data than (int)getDigits.
• Add getString
• Throw a domain exception in case of invalid data in getAlnum, getAlpha, getDigits, getBoolean, getInteger, getString. (ie filter returning false). BadRequestException for InputBag and LogicException for the generic ParameterBag.

My take on this entire thing is that we should validate rather than convert any of the inputs.

If I use getInt() is because I expect an int, if I don't get one, I don't want Symfony to guess one, but to tell me it wasn't a string.

// let's pretend that var is "2"
$request->query->getString('var', "5"); // returns "2"
$request->query->getInt('var', 5); // returns 2

// now let's pretend that var is "two"
$request->query->getString('var', "5"); // returns "two"
$request->query->getInt('var', 5); // returns 5 because "two" doesn't pass a ctype_digit() check

and so on for the other methods, validate and return the expected type if it matches, otherwise return the default value.

Additional getters could be considered, like getNumericString() for those working with bc_* functions. It would retourn only if the string is_numeric.

I'm open to working on this as I feel it's one of the biggest confusing, and, in the case of getInt(), sometimes dangerous behavior in the parameter bags.

Continued in #48548 #48525
Thank you @SHTIKOV for pushing this forward!

I think you mean #48525