Use floor() instead of round() in rest_is_integer()#11893
Use floor() instead of round() in rest_is_integer()#11893siliconforks wants to merge 16 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Change suggested by @westonruter and Claude Opus 4.7.
…is_integer()
Expands the data_rest_is_integer provider to cover values that must keep
their historical "integer-like" result and that round-trip cleanly through
the (int) cast performed by rest_sanitize_value_from_schema() for the
'integer' type:
- Integer-valued floats and decimal strings (1.0, 5.0, '1.0', '5.0').
- Scientific-notation values resolving to an integer (1.5e3, '1.5e3', '15e2').
- Signed canonical integer strings ('+5', '-5').
- Non-numeric/non-string scalars (false, null) remain rejected.
- Large-magnitude integer strings, including a negative one and 2 ** 62.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends test_rest_is_integer() so that every value treated as integer-like is also run through rest_sanitize_value_from_schema() for the 'integer' type and asserted, via assertSame(), to produce the expected integer. This keeps validation and sanitization in agreement and guards against the (int) cast munging a value that rest_is_integer() reports as valid. The data provider gains an optional third element holding the expected sanitized integer for each integer-like value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clarifies in the docblock that rest_is_integer() reports whether a value represents an integer but does not guarantee it fits in a native PHP integer: values beyond PHP_INT_MAX are still treated as integer-like even though the (int) cast used during sanitization clamps them, and a numeric value whose fractional part is too large to be represented as a float (greater than 2 ** 53) is likewise reported as integer-like. Both behaviors predate this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a comment clarifying that comparing a float to its own floor() with strict equality is an exact "does this have a fractional part?" test, not the unreliable "are two separately-computed floats equal" comparison that float equality is usually warned against. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into 65271-floor
| null, | ||
| ), | ||
|
|
||
| // The following values failed with PHP 8.4 when rest_is_integer() used round() in its implementation. |
There was a problem hiding this comment.
It's a bit confusing now what values this comment actually applies to. Originally there were 4 values following the comment, and the comment was intended to refer to all of them. You (and Claude) added a 5th and a 6th value in 7a07f03. The comment still makes sense for the 5th value ('-4503599627370498'). However, it doesn't make sense for the 6th value ('4611686018427387904') - that value would be correctly identified as an integer even with the old implementation that used round().
You could just delete the comment; it's not critical information, although I think it is useful to point out the kinds of values that weren't working with the old implementation.
|
@westonruter I beleive this PR logically solves the problem in hand. Although my personal preference would still be a more simpler and straightforward regex approach like in #11883. That is predictable and dependency free (core php changes don't affect the behaviour of the function). It's also more maintainable and easy to extend/change the ruleset (if ever needed). I'd also propose to not let values pass which should not be passed like unrealistic scientific notations, since I think it was an unintended side effect of the original implementation. That being said, I'm open to any approach the community decides upon. I'm just sharing my opinion. |
3 participants
This fixes a problem with large integers causing
rest_is_integer()to returnfalse.It also adds some tests for large integers and a few other things that did not have any existing tests (negative floats, scientific notation).
To run the test suite:
Trac ticket: https://core.trac.wordpress.org/ticket/65271
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Opus 4.7
Used for: @westonruter said that Claude Opus 4.7 made suggestions to add separate branches for
intvalues andstringintegers; these looked fine to me, so I added them.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.