Fix message overriding bug in NestedArrayFormatter

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<Result> $children
+     *
+     * @return array{0: array<array{key: string|int, child: Result}>, 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<string|int, mixed> $messages
+     * @param array<string|int, mixed> $templates
+     *
+     * @return array<string|int, mixed>
+     */
+    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<string|int, array<string>> $templates
+     *
+     * @return array<string>|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;
+    }
 }

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',
             ],
         ]),

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',
         ]),
 ));

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',
         ]),

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,15 +309,17 @@
           - `.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',
             0 => '`.0.my_int` must be present',
             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',
             ],
         ]),
 ));

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<string, array{0: Result, 1: array<string, mixed>, 2?: array<string, mixed>}> */
+    /** @return array<string, array{0: Result, 1: array<int|string, mixed>, 2?: array<int|string, mixed>}> */
     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',
+                ],
+            ],
         ];
     }
 }