-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
[PHP 8] Add a custom round method to workaround an incompatibility of the buil-in function #27830
[PHP 8] Add a custom round method to workaround an incompatibility of the buil-in function #27830
Conversation
There's a number of places in the WooCommerce codebase where the built-in function 'round' is executed passing a non-numeric value (not a number and not a string that can be parsed as a number), for example round(''). In PHP 7 this yields a value of 0, but in PHP 8 this throws an error. This commit adds a 'NumberUtil' class with a static 'round' method, this method checks if the passed value is numeric and if so it just executes the built-in function, otherwise it returns 0. And all the calls to 'round' in the codebase are replaced with 'NumberUtil::round'.
/** | ||
* A class of utilities for dealing with numbers. | ||
*/ | ||
final class NumberUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just Number
since it's under the Utilities
namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already discussed this at some point. It's better to have explicit class names for readability, and the name Number
seems to suggest that the class represents a number, which isn't the case. Also StringUtil
already exists, and in that particular case String
couldn't be used because it's a PHP reserved word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment, other than that looks good!
- Passing a string that represents a number but has spaces (e.g. ' 1 ') now works as expected (the number is properly interpreted) - Passing the boolean true now returns 1, not 0 - Passing an object throws an error, instead of returning 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
All Submissions:
Changes proposed in this Pull Request:
There's a number of places in the WooCommerce codebase where the built-in function
round
is executed passing a non-numeric value (not a number and not a string that can be parsed as a number), for exampleround('')
when passing the value of a setting that has not been initialized. In PHP 7 this yields a value of 0, but in PHP 8 this throws an error.This commit adds a
NumberUtil
class with a staticround
method, this method checks if the passed value is numeric and if so it just executes the built-in function right away, otherwise it appliesfloatval
to the value before passing it toround
. And all the calls to 'round' in the codebase are replaced withNumberUtil::round
.How to test the changes in this Pull Request:
Try to create a new order from the admin area.
Other information:
Changelog entry