[Validator] Fix precision issue regarding floats and DivisibleBy constraint #28228
Conversation
|
Looks great. 👍 |
|
The sprintf does rounding up to the 12th decimal, which means it's like your epsilon comparison, but also that is copes with the scale of the number, thanks to using the scientific notation for the rounding. |
| @@ -23,7 +23,11 @@ class DivisibleByValidator extends AbstractComparisonValidator | |||
| */ | |||
| protected function compareValues($value1, $value2) | |||
| { | |||
| return (float) 0 === fmod($value1, $value2); | |||
| if (!$value1 = fmod($value1, $value2)) { | |||
There was a problem hiding this comment.
Is the $value1= assignment important?
There was a problem hiding this comment.
it is, so that the check just below uses the rest returned by fmod()
nicolas-grekas
force-pushed
the
improve-divisible-by
branch
from
df58c7d
to
d094d4f
Compare
Yeah, but afterwards we still convert both to a float and do a direct comparison. So 👍 for the PR, as it is simpler and more readable |
nicolas-grekas
force-pushed
the
improve-divisible-by
branch
from
d094d4f
to
ae04b48
Compare
|
I improved precision a bit for integers and rounded both operands to the 12th decimal for floats. |
|
Looks great, thank you! 👍 |
|
Thank you @apfelbox. |
…ibleBy constraint (apfelbox) This PR was merged into the 4.2-dev branch. Discussion ---------- [Validator] Fix precision issue regarding floats and DivisibleBy constraint | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Alternative to #28212 Commits ------- ae04b48 [Validator] Fix precision issue regarding floats and DivisibleBy constraint
|
@nicolas-grekas but seems that still didn't works. |
|
@ERuban Can you please open a new issue if you think that things still need to be improved? Comments on already merged PRs are likely to get lost. |
Alternative to #28212