From 987713430f92ef5cdf42cb50d811537148e7d5ae Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:09:14 +0200 Subject: [PATCH 1/9] Fix bug when using multipleOf values with decimal places --- src/Schema/Type/Number.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Schema/Type/Number.php b/src/Schema/Type/Number.php index e52c6ab..e60d66b 100644 --- a/src/Schema/Type/Number.php +++ b/src/Schema/Type/Number.php @@ -50,7 +50,9 @@ public function validate(mixed $value, callable $fail): void } } - if ($this->multipleOf !== null && $value % $this->multipleOf !== 0) { + // Divide the value by multipleOf instead of using the modulo operator to avoid bugs when using a multipleOf + // that has decimal places. (Since the modulo operator converts the multipleOf to int) + if ($this->multipleOf !== null && $value/$this->multipleOf !== (float) round($value/$this->multipleOf)) { $fail(sprintf('must be a multiple of %d', $this->multipleOf)); } } From bfbff54c3f9d8df469c67567761052fe1954d0af Mon Sep 17 00:00:00 2001 From: bertramakers Date: Tue, 8 Oct 2024 08:09:35 +0000 Subject: [PATCH 2/9] Run Prettier --- src/Schema/Type/Number.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Schema/Type/Number.php b/src/Schema/Type/Number.php index e60d66b..5c402ad 100644 --- a/src/Schema/Type/Number.php +++ b/src/Schema/Type/Number.php @@ -52,7 +52,10 @@ public function validate(mixed $value, callable $fail): void // Divide the value by multipleOf instead of using the modulo operator to avoid bugs when using a multipleOf // that has decimal places. (Since the modulo operator converts the multipleOf to int) - if ($this->multipleOf !== null && $value/$this->multipleOf !== (float) round($value/$this->multipleOf)) { + if ( + $this->multipleOf !== null && + $value / $this->multipleOf !== (float) round($value / $this->multipleOf) + ) { $fail(sprintf('must be a multiple of %d', $this->multipleOf)); } } From dabc1479bfbb6c5c8b145d086e1d9638880b9b1b Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:10:48 +0200 Subject: [PATCH 3/9] Allow resetting multipleOf back to null --- src/Schema/Type/Number.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Schema/Type/Number.php b/src/Schema/Type/Number.php index 5c402ad..fe19fdb 100644 --- a/src/Schema/Type/Number.php +++ b/src/Schema/Type/Number.php @@ -90,7 +90,7 @@ public function maximum(?float $maximum, bool $exclusive = false): static public function multipleOf(?float $number): static { - if ($number <= 0) { + if ($number !== null && $number <= 0) { throw new InvalidArgumentException('multipleOf must be a positive number'); } From 8d28bdcd1a6ee64a9bba53cedb4809fbe2018efe Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:14:31 +0200 Subject: [PATCH 4/9] Add tests for multipleOf bugs --- tests/unit/NumberTest.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/unit/NumberTest.php b/tests/unit/NumberTest.php index 72c7755..4738dfa 100644 --- a/tests/unit/NumberTest.php +++ b/tests/unit/NumberTest.php @@ -40,6 +40,10 @@ public static function validationProvider(): array [Number::make()->maximum(10, exclusive: true), 10, false], [Number::make()->multipleOf(2), 1, false], [Number::make()->multipleOf(2), 2, true], + [Number::make()->multipleOf(0.01), 100, true], + [Number::make()->multipleOf(0.01), 100.5, true], + [Number::make()->multipleOf(0.01), 100.56, true], + [Number::make()->multipleOf(0.01), 100.567, false], ]; } @@ -56,4 +60,17 @@ public function test_validation(Type $type, mixed $value, bool $valid) $type->validate($value, $fail); } + + public function test_multipleOf_reset(): void + { + $number = Number::make() + ->multipleOf(2) + ->multipleOf(null); + + $fail = $this->createMock(MockedCaller::class) + ->expects($this->never()) + ->method('__invoke'); + + $number->validate(5, $fail); + } } From 76014441fda551e3a929942be11742450ee70140 Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:20:00 +0200 Subject: [PATCH 5/9] Fix broken test --- tests/unit/NumberTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/NumberTest.php b/tests/unit/NumberTest.php index 4738dfa..d2444ec 100644 --- a/tests/unit/NumberTest.php +++ b/tests/unit/NumberTest.php @@ -67,8 +67,8 @@ public function test_multipleOf_reset(): void ->multipleOf(2) ->multipleOf(null); - $fail = $this->createMock(MockedCaller::class) - ->expects($this->never()) + $fail = $this->createMock(MockedCaller::class); + $fail->expects($this->never()) ->method('__invoke'); $number->validate(5, $fail); From ccd5d6358c2ce7708ffa0e6ce9b8f4b2979d1b9e Mon Sep 17 00:00:00 2001 From: bertramakers Date: Tue, 8 Oct 2024 08:20:20 +0000 Subject: [PATCH 6/9] Run Prettier --- tests/unit/NumberTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/unit/NumberTest.php b/tests/unit/NumberTest.php index d2444ec..ef7828a 100644 --- a/tests/unit/NumberTest.php +++ b/tests/unit/NumberTest.php @@ -68,8 +68,7 @@ public function test_multipleOf_reset(): void ->multipleOf(null); $fail = $this->createMock(MockedCaller::class); - $fail->expects($this->never()) - ->method('__invoke'); + $fail->expects($this->never())->method('__invoke'); $number->validate(5, $fail); } From 15f65a29825913f705a659b495a0e412e1f2ed62 Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:21:30 +0200 Subject: [PATCH 7/9] Remove unnecessary cast to float round() always returns a float --- src/Schema/Type/Number.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Schema/Type/Number.php b/src/Schema/Type/Number.php index fe19fdb..6f534db 100644 --- a/src/Schema/Type/Number.php +++ b/src/Schema/Type/Number.php @@ -54,7 +54,7 @@ public function validate(mixed $value, callable $fail): void // that has decimal places. (Since the modulo operator converts the multipleOf to int) if ( $this->multipleOf !== null && - $value / $this->multipleOf !== (float) round($value / $this->multipleOf) + $value / $this->multipleOf !== round($value / $this->multipleOf) ) { $fail(sprintf('must be a multiple of %d', $this->multipleOf)); } From 761c47a675c4d644eb80a4e98de02e44109b22b4 Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:32:02 +0200 Subject: [PATCH 8/9] Make sure result of division is always a float --- src/Schema/Type/Number.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Schema/Type/Number.php b/src/Schema/Type/Number.php index 6f534db..4490dbd 100644 --- a/src/Schema/Type/Number.php +++ b/src/Schema/Type/Number.php @@ -52,9 +52,12 @@ public function validate(mixed $value, callable $fail): void // Divide the value by multipleOf instead of using the modulo operator to avoid bugs when using a multipleOf // that has decimal places. (Since the modulo operator converts the multipleOf to int) + // Note that dividing two integers returns another integer if the result is a whole number. So to make the + // comparison work at all times we need to cast the result to float. Casting both to integer will not work + // as intended since then the result of the division would also be rounded. if ( $this->multipleOf !== null && - $value / $this->multipleOf !== round($value / $this->multipleOf) + (float) ($value / $this->multipleOf) !== round($value / $this->multipleOf) ) { $fail(sprintf('must be a multiple of %d', $this->multipleOf)); } From 16468fd126077a8e37057aacc78e0e0d653270fe Mon Sep 17 00:00:00 2001 From: Bert Ramakers Date: Tue, 8 Oct 2024 10:59:56 +0200 Subject: [PATCH 9/9] Fix incorrect formatting of multipleOf in error message - `%d` casts the multipleOf to a whole number even if its a float with decimals - `%f` would add unnecessary decimals, e.g. `0.01` would become `0.010000` - `%s` maintains the same number of decimals, e.g. `0.01` stays `0.01` --- src/Schema/Type/Number.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Schema/Type/Number.php b/src/Schema/Type/Number.php index 4490dbd..978d70a 100644 --- a/src/Schema/Type/Number.php +++ b/src/Schema/Type/Number.php @@ -59,7 +59,7 @@ public function validate(mixed $value, callable $fail): void $this->multipleOf !== null && (float) ($value / $this->multipleOf) !== round($value / $this->multipleOf) ) { - $fail(sprintf('must be a multiple of %d', $this->multipleOf)); + $fail(sprintf('must be a multiple of %s', $this->multipleOf)); } }