Skip to content

round(): Validate the rounding mode #12252

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

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Conversation

TimWolla
Copy link
Member

No description provided.

Comment on lines +338 to +348
switch (mode) {
case PHP_ROUND_HALF_UP:
case PHP_ROUND_HALF_DOWN:
case PHP_ROUND_HALF_EVEN:
case PHP_ROUND_HALF_ODD:
break;
default:
zend_argument_value_error(3, "must be a valid rounding mode (PHP_ROUND_*)");
RETURN_THROWS();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously an invalid rounding mode was considered to be equal to PHP_ROUND_HALF_UP. This is no longer the case since #12220. Making this a ValueError would be correct, but possibly a little steep. Shall I update this to a E_WARNING + change mode to PHP_ROUND_HALF_UP within the switch()? I don't want to update the rounding helper to handle unknown cases to keep the function clean.

Copy link
Member

Choose a reason for hiding this comment

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

I think having it directly a ValueError is fine IMHO, just add an entry in UPGRADING

@TimWolla TimWolla requested a review from Girgias September 21, 2023 12:54
@staabm
Copy link
Contributor

staabm commented Sep 21, 2023

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Aside, the precision arg seems to be truncating values, maybe as a follow-up this should do the usual boundary checks and throw a value error, or make the _php_math_round() take a zend_long instead of an int ?

Comment on lines +338 to +348
switch (mode) {
case PHP_ROUND_HALF_UP:
case PHP_ROUND_HALF_DOWN:
case PHP_ROUND_HALF_EVEN:
case PHP_ROUND_HALF_ODD:
break;
default:
zend_argument_value_error(3, "must be a valid rounding mode (PHP_ROUND_*)");
RETURN_THROWS();
}

Copy link
Member

Choose a reason for hiding this comment

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

I think having it directly a ValueError is fine IMHO, just add an entry in UPGRADING

@TimWolla TimWolla merged commit 3d857d5 into php:master Sep 22, 2023
@TimWolla TimWolla deleted the round-validate-mode branch September 22, 2023 15:32
@TimWolla
Copy link
Member Author

Aside, the precision arg seems to be truncating values, maybe as a follow-up this should do the usual boundary checks and throw a value error, or make the _php_math_round() take a zend_long instead of an int ?

Any values with more than 2 digits are useless anyway. It probably makes sense to validate this, but first we need to establish how many digits are actually meaningful. There some open PRs around this by @SakiTakamachi.

@SakiTakamachi
Copy link
Member

Yes, I thought about these problems and finally found what I personally think is the best solution.
#12268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants