From cd96d0136421583733bdef5b2508d3d2d3caf8d3 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 | 121 +++++++++++++++--- 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, 192 insertions(+), 24 deletions(-) diff --git a/library/Message/Formatter/NestedArrayFormatter.php b/library/Message/Formatter/NestedArrayFormatter.php index 936432c5..250dc071 100644 --- a/library/Message/Formatter/NestedArrayFormatter.php +++ b/library/Message/Formatter/NestedArrayFormatter.php @@ -13,8 +13,11 @@ use Respect\Validation\Message\ArrayFormatter; 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,26 +28,24 @@ final readonly class NestedArrayFormatter implements ArrayFormatter */ 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; - } - - $messages[$key] = current($messages[$key]); - } + $hasString, + ), + [], + ); if (count($messages) > 1) { return ['__root__' => $renderer->render($result, $templates)] + $messages; @@ -52,4 +53,92 @@ final readonly class NestedArrayFormatter implements ArrayFormatter 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; + } + + $mapped[] = ['key' => $key, 'child' => $child]; + } + + 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 d6f220ad..ac8f517f 100644 --- a/tests/feature/Issues/Issue1289Test.php +++ b/tests/feature/Issues/Issue1289Test.php @@ -60,7 +60,10 @@ test('https://github.com/Respect/Validation/issues/1289', catchAll( ->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 cae865e7..fbd5190f 100644 --- a/tests/feature/Issues/Issue1376Test.php +++ b/tests/feature/Issues/Issue1376Test.php @@ -29,7 +29,10 @@ test('https://github.com/Respect/Validation/issues/1376', catchAll( '__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 b5627f8a..aa03288a 100644 --- a/tests/feature/Validators/AttributesTest.php +++ b/tests/feature/Validators/AttributesTest.php @@ -57,7 +57,10 @@ test('Multiple attributes, all failed', catchAll( ->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 427f42ab..58cad575 100644 --- a/tests/feature/Validators/EachTest.php +++ b/tests/feature/Validators/EachTest.php @@ -283,13 +283,21 @@ test('Chained wrapped rule', catchAll( 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 @@ test('Multiple nested rules', catchAll( - `.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 @@ test('Multiple nested rules', catchAll( 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 1ba18d4c..44dc599d 100644 --- a/tests/unit/Message/Formatter/NestedArrayFormatterTest.php +++ b/tests/unit/Message/Formatter/NestedArrayFormatterTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\Attributes\CoversClass; 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 @@ final class NestedArrayFormatterTest extends TestCase 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 @@ final class NestedArrayFormatterTest extends TestCase '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', + ], + ], ]; } }