round(): Validate the rounding mode#12252
Conversation
| 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(); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think having it directly a ValueError is fine IMHO, just add an entry in UPGRADING
96eb2f4 to
7dbdb22
Compare
|
fwiw, this error is reported by PHPStan since phpstan/phpstan-src#1126 |
Girgias
left a comment
There was a problem hiding this comment.
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 ?
| 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(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think having it directly a ValueError is fine IMHO, just add an entry in UPGRADING
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. |
|
Yes, I thought about these problems and finally found what I personally think is the best solution. |
No description provided.