Handle more collision scenarios on NestedArrayFormatter

alganet · alganet · commit 1b584d88a382 · 2026-02-02T20:21:14.000Z
See #1668 for more info. There is a new test "Deep name collision" on AllOfTest that exemplifies the kind of collision this change solves. Some extra `__root__` keys needed to be added to a select few other scenarios for consistency.
diff --git a/src/Message/Formatter/NestedArrayFormatter.php b/src/Message/Formatter/NestedArrayFormatter.php
@@ -15,7 +15,7 @@
 use Respect\Validation\Message\Renderer;
 use Respect\Validation\Result;
 
-use function array_reduce;
+use function array_values;
 use function count;
 use function current;
 use function is_array;
@@ -31,25 +31,30 @@
     public function format(Result $result, Renderer $renderer, array $templates): array
     {
         if ($result->children === []) {
-            return [$this->getKey($result) => $renderer->render($result, $templates)];
+            return [$result->path->value ?? $result->id->value => $renderer->render($result, $templates)];
         }
 
-        [$children, $hasString] = $this->prepareChildren($result->children);
+        $hasStringKey = false;
+        foreach ($result->children as $child) {
+            if (!is_numeric($child->path->value ?? $child->id->value)) {
+                $hasStringKey = true;
+                break;
+            }
+        }
 
-        $messages = array_reduce(
-            $children,
-            fn(array $messages, array $item) => $this->appendMessage(
+        $messages = [];
+        $childCount = count($result->children);
+        foreach ($result->children as $child) {
+            $messages = $this->formatChild(
                 $messages,
-                $item['key'],
-                $item['child'],
+                $child,
                 $renderer,
                 $templates,
-                $hasString,
+                $hasStringKey,
                 $result,
-                isParentVisible: count($children) > 1,
-            ),
-            [],
-        );
+                $childCount > 1,
+            );
+        }
 
         if (count($messages) > 1) {
             return ['__root__' => $renderer->render($result, $templates)] + $messages;
@@ -58,98 +63,92 @@ public function format(Result $result, Renderer $renderer, array $templates): ar
         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;
-            }
-
-            $mapped[] = ['key' => $key, 'child' => $child];
-        }
-
-        return [$mapped, $hasString];
-    }
-
     /**
      * @param array<string|int, mixed> $messages
      * @param array<string|int, mixed> $templates
      *
      * @return array<string|int, mixed>
      */
-    private function appendMessage(
+    private function formatChild(
         array $messages,
-        string|int $key,
         Result $child,
         Renderer $renderer,
         array $templates,
-        bool $hasString,
+        bool $hasStringKey,
         Result $parent,
-        bool $isParentVisible,
+        bool $hasMultipleChildren,
     ): array {
-        if ($hasString && is_numeric($key)) {
-            $key = $child->id->value;
-        }
+        $rawKey = $child->path->value ?? $child->id->value;
+        $normalizedKey = $hasStringKey && is_numeric($rawKey) ? $child->id->value : $rawKey;
 
-        $message = $this->renderChild($child, $renderer, $templates, $parent, $isParentVisible);
+        $formatted = $this->format(
+            $hasMultipleChildren && $child->name === $parent->name ? $child->withoutName() : $child,
+            $renderer,
+            $templates,
+        );
 
-        if (!$hasString) {
-            $messages[] = $message;
+        $childMessage = count($formatted) === 1 ? current($formatted) : $formatted;
 
-            return $messages;
+        if (is_array($childMessage) && count($childMessage) > 1 && !isset($childMessage['__root__'])) {
+            $childMessage = ['__root__' => $renderer->render($child, $templates)] + $childMessage;
         }
 
-        if (isset($messages[$key])) {
-            if (!is_array($messages[$key])) {
-                $messages[$key] = [$messages[$key]];
-            }
+        if (!$hasStringKey) {
+            $messages[] = $childMessage;
 
-            $messages[$key][] = $message;
+            return $messages;
+        }
+
+        if (!isset($messages[$normalizedKey])) {
+            $messages[$normalizedKey] = $childMessage;
 
             return $messages;
         }
 
-        $messages[$key] = $message;
+        if ($child->path !== null) {
+            return $this->mergeWithExistingPath($messages, $normalizedKey, $childMessage);
+        }
 
-        return $messages;
+        return $this->flattenToIndexedList($messages, $childMessage, $parent, $renderer, $templates);
     }
 
     /**
-     * @param array<string|int, array<string>> $templates
+     * @param array<string|int, mixed> $messages
+     * @param array<string|int, mixed>|string $childMessage
      *
-     * @return array<string>|string
+     * @return array<string|int, mixed>
      */
-    private function renderChild(
-        Result $child,
-        Renderer $renderer,
-        array $templates,
-        Result $parent,
-        bool $isParentVisible,
-    ): array|string {
-        $formatted = $this->format(
-            $isParentVisible && $child->name === $parent->name ? $child->withoutName() : $child,
-            $renderer,
-            $templates,
-        );
+    private function mergeWithExistingPath(array $messages, string|int $key, array|string $childMessage): array
+    {
+        if (is_array($messages[$key])) {
+            if (isset($messages[$key]['__root__'])) {
+                $messages[$key] = [$messages[$key]];
+            }
 
-        if (count($formatted) === 1) {
-            return current($formatted);
+            $messages[$key][] = $childMessage;
+        } else {
+            $messages[$key] = [$messages[$key], $childMessage];
         }
 
-        return $formatted;
+        return $messages;
     }
 
-    private function getKey(Result $result): string|int
-    {
-        return $result->path->value ?? $result->id->value;
+    /**
+     * @param array<string|int, mixed> $messages
+     * @param array<string|int, mixed>|string $childMessage
+     * @param array<string|int, mixed> $templates
+     *
+     * @return array<string|int, mixed>
+     */
+    private function flattenToIndexedList(
+        array $messages,
+        array|string $childMessage,
+        Result $parent,
+        Renderer $renderer,
+        array $templates,
+    ): array {
+        $parentMessage = $messages['__root__'] ?? $renderer->render($parent, $templates);
+
+        return ['__root__' => $parentMessage] + array_values($messages) + [count($messages) => $childMessage];
     }
 }
diff --git a/tests/feature/Issues/Issue1289Test.php b/tests/feature/Issues/Issue1289Test.php
@@ -63,8 +63,9 @@
             0 => [
                 '__root__' => '`.0` must pass the rules',
                 'default' => [
-                    '`.0.default` must be a string',
-                    '`.0.default` must be a boolean',
+                    '__root__' => '`.0.default` must pass one of the rules',
+                    0 => '`.0.default` must be a string',
+                    1 => '`.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
@@ -32,8 +32,9 @@
             'title' => '`.title` must be present',
             'description' => '`.description` must be present',
             'author' => [
-                '`.author` must be an integer',
-                'The length of `.author` must be between 1 and 2',
+                '__root__' => '`.author` must pass all the rules',
+                0 => '`.author` must be an integer',
+                1 => 'The length of `.author` must be between 1 and 2',
             ],
             'user' => '`.user` must be present',
         ]),
diff --git a/tests/feature/Issues/Issue1668Test.php b/tests/feature/Issues/Issue1668Test.php
@@ -0,0 +1,130 @@
+<?php
+
+/*
+ * SPDX-License-Identifier: MIT
+ * SPDX-FileCopyrightText: (c) Respect Project Contributors
+ * SPDX-FileContributor: Alexandre Gomes Gaigalas <alganet@gmail.com>
+ */
+
+declare(strict_types=1);
+
+test('https://github.com/Respect/Validation/issues/1668', catchAll(
+    fn() => v::allOf(
+        v::named('alpha', v::allOf(
+            v::contains('quick'),
+            v::contains('fox'),
+        )),
+        v::named('zeta', v::allOf(
+            v::contains('lorem'),
+            v::contains('ipsum'),
+        )),
+    )->assert('foo bar baz'),
+    fn(string $message, string $fullMessage, array $messages) => expect()
+        ->and($message)->toBe('alpha must contain "quick"')
+        ->and($fullMessage)->toBe(<<<'FULL_MESSAGE'
+            - "foo bar baz" must pass all the rules
+              - alpha must pass all the rules
+                - "foo bar baz" must contain "quick"
+                - "foo bar baz" must contain "fox"
+              - zeta must pass all the rules
+                - "foo bar baz" must contain "lorem"
+                - "foo bar baz" must contain "ipsum"
+            FULL_MESSAGE)
+        ->and($messages)->toBe([
+            '__root__' => '"foo bar baz" must pass all the rules',
+            0 => [
+                '__root__' => 'alpha must pass all the rules',
+                0 => '"foo bar baz" must contain "quick"',
+                1 => '"foo bar baz" must contain "fox"',
+            ],
+            1 => [
+                '__root__' => 'zeta must pass all the rules',
+                0 => '"foo bar baz" must contain "lorem"',
+                1 => '"foo bar baz" must contain "ipsum"',
+            ],
+        ]),
+));
+
+test('https://github.com/Respect/Validation/issues/1668 #2', catchAll(
+    fn() => v::allOf(
+        v::allOf(
+            v::contains('quick'),
+            v::contains('fox'),
+        ),
+        v::allOf(
+            v::contains('lorem'),
+            v::contains('ipsum'),
+        ),
+    )->assert('foo bar baz'),
+    fn(string $message, string $fullMessage, array $messages) => expect()
+        ->and($message)->toBe('"foo bar baz" must contain "quick"')
+        ->and($fullMessage)->toBe(<<<'FULL_MESSAGE'
+            - "foo bar baz" must pass all the rules
+              - "foo bar baz" must pass all the rules
+                - "foo bar baz" must contain "quick"
+                - "foo bar baz" must contain "fox"
+              - "foo bar baz" must pass all the rules
+                - "foo bar baz" must contain "lorem"
+                - "foo bar baz" must contain "ipsum"
+            FULL_MESSAGE)
+        ->and($messages)->toBe([
+            '__root__' => '"foo bar baz" must pass all the rules',
+            0 => [
+                '__root__' => '"foo bar baz" must pass all the rules',
+                0 => '"foo bar baz" must contain "quick"',
+                1 => '"foo bar baz" must contain "fox"',
+            ],
+            1 => [
+                '__root__' => '"foo bar baz" must pass all the rules',
+                0 => '"foo bar baz" must contain "lorem"',
+                1 => '"foo bar baz" must contain "ipsum"',
+            ],
+        ]),
+));
+
+
+test('https://github.com/Respect/Validation/issues/1668 #3', catchAll(
+    fn() => v::named('match_making', v::allOf(
+        v::key('couple', v::named('movies', v::allOf(
+            v::key('she', v::named('vampire', v::contains('Twilight'))),
+            v::key('he', v::named('heroes', v::contains('Avengers'))),
+        ))),
+        v::key('couple', v::named('food', v::allOf(
+            v::key('she', v::named('comfort', v::contains('mac & cheese'))),
+            v::key('he', v::named('junk', v::contains('nachos'))),
+        ))),
+    ))->assert(['couple' => ['she' => 'Gilmore Girls, soup', 'he' => 'Batman, pizza']], [
+        'match_making' => 'Match Making',
+        'movies' => 'Cinema',
+        'food' => 'Cousine',
+        'vampire' => 'She must like vampire movies',
+        'heroes' => 'He must like hero movies',
+        'comfort' => 'She must like comfort food',
+        'junk' => 'He must like junk food',
+    ]),
+    fn(string $message, string $fullMessage, array $messages) => expect()
+        ->and($message)->toBe('She must like vampire movies')
+        ->and($fullMessage)->toBe(<<<'FULL_MESSAGE'
+            - Match Making
+              - Cinema
+                - She must like vampire movies
+                - He must like hero movies
+              - Cousine
+                - She must like comfort food
+                - He must like junk food
+            FULL_MESSAGE)
+        ->and($messages)->toBe([
+            'couple' => [
+                0 => [
+                    '__root__' => 'Cinema',
+                    'she' => 'She must like vampire movies',
+                    'he' => 'He must like hero movies',
+                ],
+                1 => [
+                    '__root__' => 'Cousine',
+                    'she' => 'She must like comfort food',
+                    'he' => 'He must like junk food',
+                ],
+            ],
+        ]),
+));
diff --git a/tests/feature/Validators/AttributesTest.php b/tests/feature/Validators/AttributesTest.php
@@ -60,8 +60,9 @@
             '__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' => [
-                '`.birthdate` must be a valid date in the format "2005-12-30"',
-                'For comparison with now, `.birthdate` must be a valid datetime',
+                '__root__' => '`.birthdate` must pass all the rules',
+                0 => '`.birthdate` must be a valid date in the format "2005-12-30"',
+                1 => '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/unit/Message/Formatter/NestedArrayFormatterTest.php b/tests/unit/Message/Formatter/NestedArrayFormatterTest.php
