Improve casting of integer routing parameters #16643
Conversation
Allow negative numbers to be cast to integers for typed routing parameters.
src/Controller/ControllerFactory.php
Outdated
| @@ -268,7 +268,7 @@ protected function coerceStringToType(string $argument, ReflectionNamedType $typ | |||
| case 'float': | |||
| return is_numeric($argument) ? (float)$argument : null; | |||
| case 'int': | |||
| return ctype_digit($argument) ? (int)$argument : null; | |||
| return ctype_digit($argument) || filter_var($argument, FILTER_VALIDATE_INT) ? (int)$argument : null; | |||
There was a problem hiding this comment.
The filter_var() check doesn't cover the ctype_digit() check too?
There was a problem hiding this comment.
filter_var doesn't validate leading 0's unless you enable octal, but that will re-interpret the value.
There was a problem hiding this comment.
Unless we have existing tests for casting strings with leading zeros to int I don't think we need to support it.
There was a problem hiding this comment.
We do, The first test checks 02 which is why filter_var() fails on its own.
There was a problem hiding this comment.
Yeah, I'm not sure we want to try and support octals long term. But its there right now 🤷
There was a problem hiding this comment.
We don't support octals. It's just casting the string to int which strips the leading 0s.
There was a problem hiding this comment.
The leading 0 handling could be interpreted as octal support though. I'm ok breaking this, it feels like a scenario that isn't going to be common in userland code.
There was a problem hiding this comment.
I'm ok with it as well. The unit tests were added to support the code we used to validate.
There was a problem hiding this comment.
Let's remove the ctype_digit() check and the test with leading zeros then.
The additional ctype_digit logic is more complex than using filter_var() and I don't think there are many useful scenarios for leading 0s as they could be ambiguous with octal representations. See discussion in #16643
Allow negative numbers to be cast to integers for typed routing parameters.
Fixes #16640