Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve casting of integer routing parameters #16643

Merged
merged 2 commits into from Jul 25, 2022
Merged

Improve casting of integer routing parameters #16643

merged 2 commits into from Jul 25, 2022

Conversation

markstory
Copy link
Member

@markstory markstory commented Jul 21, 2022

Allow negative numbers to be cast to integers for typed routing parameters.

Fixes #16640

Allow negative numbers to be cast to integers for typed routing
parameters.
@markstory markstory added this to the 4.4.3 milestone Jul 21, 2022
@@ -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;
Copy link
Member

@ADmad ADmad Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter_var() check doesn't cover the ctype_digit() check too?

Copy link
Member

@othercorey othercorey Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter_var doesn't validate leading 0's unless you enable octal, but that will re-interpret the value.

Copy link
Member

@ADmad ADmad Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we have existing tests for casting strings with leading zeros to int I don't think we need to support it.

Copy link
Member

@othercorey othercorey Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, The first test checks 02 which is why filter_var() fails on its own.

Copy link
Member

@ADmad ADmad Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Member Author

@markstory markstory Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure we want to try and support octals long term. But its there right now 🤷

Copy link
Member

@othercorey othercorey Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support octals. It's just casting the string to int which strips the leading 0s.

Copy link
Member Author

@markstory markstory Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@othercorey othercorey Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with it as well. The unit tests were added to support the code we used to validate.

Copy link
Member

@ADmad ADmad Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
ADmad
ADmad approved these changes Jul 25, 2022
@markstory markstory merged commit f95953c into 4.x Jul 25, 2022
12 of 13 checks passed
@markstory markstory deleted the fix-16640 branch Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants