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

[9.x] Ensure decimal rule handles large values #45693

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Jan 18, 2023

The validator does not currently handle arbitrary length floats when comparing the size of decimal numbers.

The following code currently passes when it should not...

Validator::validate([
    'foo' => '1.88888888888888888888'
], [
    'foo' => [
        'decimal:20',
        'min:1.88888888888888888889',
    ],
]);

// [
//    "foo" => "1.88888888888888888888",
//  ]

This PR leans on brick/math further whenever we are comparing a value's size.

@@ -17,6 +17,7 @@
"require": {
"php": "^8.0.2",
"ext-json": "*",
"brick/math": "^0.10.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was missed when fixing the decimal model cast.

@timacdonald timacdonald changed the title Ensure decimal rule handles large values [9.x] Ensure decimal rule handles large values Jan 18, 2023
@timacdonald timacdonald marked this pull request as ready for review January 23, 2023 00:14
Comment on lines -2324 to +2326
* @return mixed
* @return int|float|string
Copy link
Member Author

Choose a reason for hiding this comment

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

Tightening this return type, as I believe it can only return these types.

@taylorotwell taylorotwell merged commit da078c0 into laravel:9.x Jan 23, 2023
@timacdonald timacdonald deleted the decimal-validation branch January 23, 2023 03:43
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.

2 participants