From 9f8c946a4514c6181969c04edfff97de2a52f4fc Mon Sep 17 00:00:00 2001 From: Alexandre Gomes Gaigalas Date: Sun, 11 Jan 2026 01:09:35 -0300 Subject: [PATCH] Fix message overriding bug in NestedArrayFormatter This commit resolves an issue where validation messages would overwrite each other when multiple validators failed on the same path or key (e.g., within an `Each` or `Key` validator). Changes to `NestedArrayFormatter`: - Implemented a merge strategy: Key collisions now result in a list of messages instead of the last message winning. - Improved handling of mixed key types: When both numeric and string keys are present (common in composite validators), numeric keys are now replaced by the validator's ID (e.g., `arrayType`, `equals`) to provide meaningful, distinct keys. - Preserved list behavior: Purely numeric key sets are treated as lists, maintaining their sequence without re-keying logic. - Refactored the class to use smaller, single-purpose methods and `array_reduce` for clarity. Tests: - Updated feature tests (`EachTest`, `AttributesTest`, etc.) to expect the full set of validation errors. - Enhanced `NestedArrayFormatterTest` with scenarios for key collisions, mixed keys, and ID substitution. --- .../Formatter/NestedArrayFormatter.php | 119 +++++++++++++++--- tests/feature/Issues/Issue1289Test.php | 5 +- tests/feature/Issues/Issue1376Test.php | 5 +- tests/feature/Validators/AttributesTest.php | 5 +- tests/feature/Validators/EachTest.php | 18 ++- .../Formatter/NestedArrayFormatterTest.php | 62 ++++++++- 6 files changed, 191 insertions(+), 23 deletions(-) diff --git a/library/Message/Formatter/NestedArrayFormatter.php b/library/Message/Formatter/NestedArrayFormatter.php index 936432c5e..250dc0719 100644 --- a/library/Message/Formatter/NestedArrayFormatter.php +++ b/library/Message/Formatter/NestedArrayFormatter.php @@ -13,8 +13,11 @@ use Respect\Validation\Message\Renderer; use Respect\Validation\Result; +use function array_reduce; use function count; use function current; +use function is_array; +use function is_numeric; final readonly class NestedArrayFormatter implements ArrayFormatter { @@ -25,31 +28,117 @@ */ public function format(Result $result, Renderer $renderer, array $templates): array { - if (count($result->children) === 0) { - return [ - $result->path->value ?? $result->id->value => $renderer->render($result, $templates), - ]; + if ($result->children === []) { + return [$this->getKey($result) => $renderer->render($result, $templates)]; } - $messages = []; - foreach ($result->children as $child) { - $key = $child->path->value ?? $child->id->value; - $messages[$key] = $this->format( - $child->withoutName(), + [$children, $hasString] = $this->prepareChildren($result->children); + + $messages = array_reduce( + $children, + fn(array $messages, array $item) => $this->appendMessage( + $messages, + $item['key'], + $item['child'], $renderer, $templates, - ); - if (count($messages[$key]) !== 1) { - continue; + $hasString, + ), + [], + ); + + if (count($messages) > 1) { + return ['__root__' => $renderer->render($result, $templates)] + $messages; + } + + return $messages; + } + + /** + * @param array $children + * + * @return array{0: array, 1: bool} + */ + private function prepareChildren(array $children): array + { + $mapped = []; + $hasString = false; + + foreach ($children as $child) { + $key = $this->getKey($child); + if (!is_numeric($key)) { + $hasString = true; } - $messages[$key] = current($messages[$key]); + $mapped[] = ['key' => $key, 'child' => $child]; } - if (count($messages) > 1) { - return ['__root__' => $renderer->render($result, $templates)] + $messages; + return [$mapped, $hasString]; + } + + /** + * @param array $messages + * @param array $templates + * + * @return array + */ + private function appendMessage( + array $messages, + string|int $key, + Result $child, + Renderer $renderer, + array $templates, + bool $hasString, + ): array { + if ($hasString && is_numeric($key)) { + $key = $child->id->value; } + $message = $this->renderChild($child, $renderer, $templates); + + if (!$hasString) { + $messages[] = $message; + + return $messages; + } + + if (isset($messages[$key])) { + if (!is_array($messages[$key])) { + $messages[$key] = [$messages[$key]]; + } + + $messages[$key][] = $message; + + return $messages; + } + + $messages[$key] = $message; + return $messages; } + + /** + * @param array> $templates + * + * @return array|string + */ + private function renderChild(Result $child, Renderer $renderer, array $templates): array|string + { + $formatted = $this->format( + $child->withoutName(), + $renderer, + $templates, + ); + + if (count($formatted) === 1) { + return current($formatted); + } + + return $formatted; + } + + private function getKey(Result $result): string|int + { + return $result->path->value ?? $result->id->value; + } } diff --git a/tests/feature/Issues/Issue1289Test.php b/tests/feature/Issues/Issue1289Test.php index d6f220ad0..ac8f517f2 100644 --- a/tests/feature/Issues/Issue1289Test.php +++ b/tests/feature/Issues/Issue1289Test.php @@ -60,7 +60,10 @@ ->and($messages)->toBe([ 0 => [ '__root__' => '`.0` must pass the rules', - 'default' => '`.0.default` must be a boolean', + 'default' => [ + '`.0.default` must be a string', + '`.0.default` must be a boolean', + ], 'description' => '`.0.description` must be a string value', ], ]), diff --git a/tests/feature/Issues/Issue1376Test.php b/tests/feature/Issues/Issue1376Test.php index cae865e76..fbd5190fe 100644 --- a/tests/feature/Issues/Issue1376Test.php +++ b/tests/feature/Issues/Issue1376Test.php @@ -29,7 +29,10 @@ '__root__' => '`stdClass { +$author="foo" }` must pass all the rules', 'title' => '`.title` must be present', 'description' => '`.description` must be present', - 'author' => 'The length of `.author` must be between 1 and 2', + 'author' => [ + '`.author` must be an integer', + 'The length of `.author` must be between 1 and 2', + ], 'user' => '`.user` must be present', ]), )); diff --git a/tests/feature/Validators/AttributesTest.php b/tests/feature/Validators/AttributesTest.php index b5627f8a0..aa03288a5 100644 --- a/tests/feature/Validators/AttributesTest.php +++ b/tests/feature/Validators/AttributesTest.php @@ -57,7 +57,10 @@ ->and($messages)->toBe([ '__root__' => '`Respect\Validation\Test\Stubs\WithAttributes { +$name="" +$birthdate="not a date" #$phone="not a phone number" + ... }` must pass the rules', 'name' => '`.name` must be defined', - 'birthdate' => 'For comparison with now, `.birthdate` must be a valid datetime', + 'birthdate' => [ + '`.birthdate` must be a valid date in the format "2005-12-30"', + 'For comparison with now, `.birthdate` must be a valid datetime', + ], 'phone' => '`.phone` must be a valid telephone number or must be null', 'email' => '`.email` must be a valid email address or must be null', ]), diff --git a/tests/feature/Validators/EachTest.php b/tests/feature/Validators/EachTest.php index 427f42abb..58cad5753 100644 --- a/tests/feature/Validators/EachTest.php +++ b/tests/feature/Validators/EachTest.php @@ -283,13 +283,21 @@ FULL_MESSAGE) ->and($messages)->toBe([ '__root__' => 'Each item in `[2, 4]` must be valid', - 0 => '`.0` must be an odd number', - 1 => '`.1` must be an odd number', + 0 => [ + '__root__' => '`.0` must pass all the rules', + 0 => '`.0` must be between 5 and 7', + 1 => '`.0` must be an odd number', + ], + 1 => [ + '__root__' => '`.1` must pass all the rules', + 0 => '`.1` must be between 5 and 7', + 1 => '`.1` must be an odd number', + ], ]), )); test('Multiple nested rules', catchAll( - fn() => v::each(v::arrayType()->key('my_int', v::intType()->odd()))->assert([['not_int' => 'wrong'], ['my_int' => 2], 'not an array']), + fn() => v::each(v::arrayType()->key('my_int', v::intType()->odd())->length(v::equals(1)))->assert([['not_int' => 'wrong'], ['my_int' => 2], 'not an array']), fn(string $message, string $fullMessage, array $messages) => expect() ->and($message)->toBe('`.0.my_int` must be present') ->and($fullMessage)->toBe(<<<'FULL_MESSAGE' @@ -301,6 +309,7 @@ - `.2` must pass all the rules - `.2` must be an array - `.2.my_int` must be present + - The length of `.2` must be equal to 1 FULL_MESSAGE) ->and($messages)->toBe([ '__root__' => 'Each item in `[["not_int": "wrong"], ["my_int": 2], "not an array"]` must be valid', @@ -308,8 +317,9 @@ 1 => '`.1.my_int` must be an odd number', 2 => [ '__root__' => '`.2` must pass all the rules', - 2 => '`.2` must be an array', + 'arrayType' => '`.2` must be an array', 'my_int' => '`.2.my_int` must be present', + 'lengthEquals' => 'The length of `.2` must be equal to 1', ], ]), )); diff --git a/tests/unit/Message/Formatter/NestedArrayFormatterTest.php b/tests/unit/Message/Formatter/NestedArrayFormatterTest.php index 1ba18d4c9..44dc599d7 100644 --- a/tests/unit/Message/Formatter/NestedArrayFormatterTest.php +++ b/tests/unit/Message/Formatter/NestedArrayFormatterTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use Respect\Validation\Message\StandardFormatter\ResultCreator; +use Respect\Validation\Path; use Respect\Validation\Result; use Respect\Validation\Test\Builders\ResultBuilder; use Respect\Validation\Test\Message\TestingMessageRenderer; @@ -37,7 +38,7 @@ public function itShouldFormatArrayMessage(Result $result, array $expected, arra self::assertSame($expected, $formatter->format($result, $renderer, $templates)); } - /** @return array, 2?: array}> */ + /** @return array, 2?: array}> */ public static function provideForArray(): array { return [ @@ -76,6 +77,65 @@ public static function provideForArray(): array '3rd' => '__3rd_original__', ], ], + 'with string key collision' => [ + (new ResultBuilder())->id('root')->template('root_msg') + ->children( + (new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path('foo'))->build(), + (new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path('foo'))->build(), + )->build(), + [ + 'foo' => ['msg1', 'msg2'], + ], + ], + 'with numeric key collision (list)' => [ + (new ResultBuilder())->id('root')->template('root_msg') + ->children( + (new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path(0))->build(), + (new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path(0))->build(), + )->build(), + [ + '__root__' => 'root_msg', + 0 => 'msg1', + 1 => 'msg2', + ], + ], + 'with mixed keys replacement' => [ + (new ResultBuilder())->id('root')->template('root_msg') + ->children( + (new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path('foo'))->build(), + (new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path(0))->build(), + )->build(), + [ + '__root__' => 'root_msg', + 'foo' => 'msg1', + 'c2' => 'msg2', + ], + ], + 'with mixed keys and ID collision' => [ + (new ResultBuilder())->id('root')->template('root_msg') + ->children( + (new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path('foo'))->build(), + (new ResultBuilder())->id('sameId')->template('msg2')->withPath(new Path(0))->build(), + (new ResultBuilder())->id('sameId')->template('msg3')->withPath(new Path(1))->build(), + )->build(), + [ + '__root__' => 'root_msg', + 'foo' => 'msg1', + 'sameId' => ['msg2', 'msg3'], + ], + ], + 'with pure numeric keys' => [ + (new ResultBuilder())->id('root')->template('root_msg') + ->children( + (new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path(10))->build(), + (new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path(20))->build(), + )->build(), + [ + '__root__' => 'root_msg', + 0 => 'msg1', + 1 => 'msg2', + ], + ], ]; } }