Ensure QuantityLimits::limit_to_multiple receives correct values to prevent fatal errors
#51451
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
|
Hi @senadir, Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. You can follow this guide to find out what good testing instructions should look like: |
| $multiple_of = (int) $this->filter_value( 1, 'multiple_of', $product ); | ||
| $minimum = (int) $this->filter_value( 1, 'minimum', $product ); | ||
| $maximum = (int) $this->filter_value( $this->get_product_quantity_limit( $product ), 'maximum', $product ); |
There was a problem hiding this comment.
This only casts, but does it check if someone returned I dunno a random abc string? This would evaluate to 0, which works fine for minimum but would break multiple and maximum
There was a problem hiding this comment.
This is true if the filter is misused. The filter docblock and passed value is int. The error logs showed that someone filtered it to return '' empty string. I guess we could add another layer of value forcing.
|
@senadir Its a bit more complicated now but I added some basic enforcing of min/max values and more explicit types on the filters. |
| // Minimum must be at least 1. | ||
| $minimum = max( $minimum, 1 ); | ||
|
|
||
| // Maximum must be at least minimum. | ||
| $maximum = max( $maximum, $minimum ); |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Ensure that filtered values are in fact integers before passing to
QuantityLimits::limit_to_multipleto prevent fatal errors.Closes #51340
How to test the changes in this Pull Request:
Should be covered by CI, but to test something that consumes the changed function:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment