Skip to content

Commit cd96d01

Browse files
alganetalganet
committed
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.
1 parent 4cc45b5 commit cd96d01

6 files changed

Lines changed: 191 additions & 23 deletions

File tree

‎library/Message/Formatter/NestedArrayFormatter.php‎

Lines changed: 104 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,11 @@
1313
use Respect\Validation\Message\Renderer;
1414
use Respect\Validation\Result;
1515

16+
use function array_reduce;
1617
use function count;
1718
use function current;
19+
use function is_array;
20+
use function is_numeric;
1821

1922
final readonly class NestedArrayFormatter implements ArrayFormatter
2023
{
@@ -25,31 +28,117 @@
2528
*/
2629
public function format(Result $result, Renderer $renderer, array $templates): array
2730
{
28-
if (count($result->children) === 0) {
29-
return [
30-
$result->path->value ?? $result->id->value => $renderer->render($result, $templates),
31-
];
31+
if ($result->children === []) {
32+
return [$this->getKey($result) => $renderer->render($result, $templates)];
3233
}
3334

34-
$messages = [];
35-
foreach ($result->children as $child) {
36-
$key = $child->path->value ?? $child->id->value;
37-
$messages[$key] = $this->format(
38-
$child->withoutName(),
35+
[$children, $hasString] = $this->prepareChildren($result->children);
36+
37+
$messages = array_reduce(
38+
$children,
39+
fn(array $messages, array $item) => $this->appendMessage(
40+
$messages,
41+
$item['key'],
42+
$item['child'],
3943
$renderer,
4044
$templates,
41-
);
42-
if (count($messages[$key]) !== 1) {
43-
continue;
45+
$hasString,
46+
),
47+
[],
48+
);
49+
50+
if (count($messages) > 1) {
51+
return ['__root__' => $renderer->render($result, $templates)] + $messages;
52+
}
53+
54+
return $messages;
55+
}
56+
57+
/**
58+
* @param array<Result> $children
59+
*
60+
* @return array{0: array<array{key: string|int, child: Result}>, 1: bool}
61+
*/
62+
private function prepareChildren(array $children): array
63+
{
64+
$mapped = [];
65+
$hasString = false;
66+
67+
foreach ($children as $child) {
68+
$key = $this->getKey($child);
69+
if (!is_numeric($key)) {
70+
$hasString = true;
4471
}
4572

46-
$messages[$key] = current($messages[$key]);
73+
$mapped[] = ['key' => $key, 'child' => $child];
4774
}
4875

49-
if (count($messages) > 1) {
50-
return ['__root__' => $renderer->render($result, $templates)] + $messages;
76+
return [$mapped, $hasString];
77+
}
78+
79+
/**
80+
* @param array<string|int, mixed> $messages
81+
* @param array<string|int, mixed> $templates
82+
*
83+
* @return array<string|int, mixed>
84+
*/
85+
private function appendMessage(
86+
array $messages,
87+
string|int $key,
88+
Result $child,
89+
Renderer $renderer,
90+
array $templates,
91+
bool $hasString,
92+
): array {
93+
if ($hasString && is_numeric($key)) {
94+
$key = $child->id->value;
5195
}
5296

97+
$message = $this->renderChild($child, $renderer, $templates);
98+
99+
if (!$hasString) {
100+
$messages[] = $message;
101+
102+
return $messages;
103+
}
104+
105+
if (isset($messages[$key])) {
106+
if (!is_array($messages[$key])) {
107+
$messages[$key] = [$messages[$key]];
108+
}
109+
110+
$messages[$key][] = $message;
111+
112+
return $messages;
113+
}
114+
115+
$messages[$key] = $message;
116+
53117
return $messages;
54118
}
119+
120+
/**
121+
* @param array<string|int, array<string>> $templates
122+
*
123+
* @return array<string>|string
124+
*/
125+
private function renderChild(Result $child, Renderer $renderer, array $templates): array|string
126+
{
127+
$formatted = $this->format(
128+
$child->withoutName(),
129+
$renderer,
130+
$templates,
131+
);
132+
133+
if (count($formatted) === 1) {
134+
return current($formatted);
135+
}
136+
137+
return $formatted;
138+
}
139+
140+
private function getKey(Result $result): string|int
141+
{
142+
return $result->path->value ?? $result->id->value;
143+
}
55144
}

‎tests/feature/Issues/Issue1289Test.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,10 @@
6060
->and($messages)->toBe([
6161
0 => [
6262
'__root__' => '`.0` must pass the rules',
63-
'default' => '`.0.default` must be a boolean',
63+
'default' => [
64+
'`.0.default` must be a string',
65+
'`.0.default` must be a boolean',
66+
],
6467
'description' => '`.0.description` must be a string value',
6568
],
6669
]),

‎tests/feature/Issues/Issue1376Test.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,10 @@
2929
'__root__' => '`stdClass { +$author="foo" }` must pass all the rules',
3030
'title' => '`.title` must be present',
3131
'description' => '`.description` must be present',
32-
'author' => 'The length of `.author` must be between 1 and 2',
32+
'author' => [
33+
'`.author` must be an integer',
34+
'The length of `.author` must be between 1 and 2',
35+
],
3336
'user' => '`.user` must be present',
3437
]),
3538
));

‎tests/feature/Validators/AttributesTest.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,10 @@
5757
->and($messages)->toBe([
5858
'__root__' => '`Respect\Validation\Test\Stubs\WithAttributes { +$name="" +$birthdate="not a date" #$phone="not a phone number" + ... }` must pass the rules',
5959
'name' => '`.name` must be defined',
60-
'birthdate' => 'For comparison with now, `.birthdate` must be a valid datetime',
60+
'birthdate' => [
61+
'`.birthdate` must be a valid date in the format "2005-12-30"',
62+
'For comparison with now, `.birthdate` must be a valid datetime',
63+
],
6164
'phone' => '`.phone` must be a valid telephone number or must be null',
6265
'email' => '`.email` must be a valid email address or must be null',
6366
]),

‎tests/feature/Validators/EachTest.php‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -283,13 +283,21 @@
283283
FULL_MESSAGE)
284284
->and($messages)->toBe([
285285
'__root__' => 'Each item in `[2, 4]` must be valid',
286-
0 => '`.0` must be an odd number',
287-
1 => '`.1` must be an odd number',
286+
0 => [
287+
'__root__' => '`.0` must pass all the rules',
288+
0 => '`.0` must be between 5 and 7',
289+
1 => '`.0` must be an odd number',
290+
],
291+
1 => [
292+
'__root__' => '`.1` must pass all the rules',
293+
0 => '`.1` must be between 5 and 7',
294+
1 => '`.1` must be an odd number',
295+
],
288296
]),
289297
));
290298

291299
test('Multiple nested rules', catchAll(
292-
fn() => v::each(v::arrayType()->key('my_int', v::intType()->odd()))->assert([['not_int' => 'wrong'], ['my_int' => 2], 'not an array']),
300+
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']),
293301
fn(string $message, string $fullMessage, array $messages) => expect()
294302
->and($message)->toBe('`.0.my_int` must be present')
295303
->and($fullMessage)->toBe(<<<'FULL_MESSAGE'
@@ -301,15 +309,17 @@
301309
- `.2` must pass all the rules
302310
- `.2` must be an array
303311
- `.2.my_int` must be present
312+
- The length of `.2` must be equal to 1
304313
FULL_MESSAGE)
305314
->and($messages)->toBe([
306315
'__root__' => 'Each item in `[["not_int": "wrong"], ["my_int": 2], "not an array"]` must be valid',
307316
0 => '`.0.my_int` must be present',
308317
1 => '`.1.my_int` must be an odd number',
309318
2 => [
310319
'__root__' => '`.2` must pass all the rules',
311-
2 => '`.2` must be an array',
320+
'arrayType' => '`.2` must be an array',
312321
'my_int' => '`.2.my_int` must be present',
322+
'lengthEquals' => 'The length of `.2` must be equal to 1',
313323
],
314324
]),
315325
));

‎tests/unit/Message/Formatter/NestedArrayFormatterTest.php‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use PHPUnit\Framework\Attributes\DataProvider;
1414
use PHPUnit\Framework\Attributes\Test;
1515
use Respect\Validation\Message\StandardFormatter\ResultCreator;
16+
use Respect\Validation\Path;
1617
use Respect\Validation\Result;
1718
use Respect\Validation\Test\Builders\ResultBuilder;
1819
use Respect\Validation\Test\Message\TestingMessageRenderer;
@@ -37,7 +38,7 @@ public function itShouldFormatArrayMessage(Result $result, array $expected, arra
3738
self::assertSame($expected, $formatter->format($result, $renderer, $templates));
3839
}
3940

40-
/** @return array<string, array{0: Result, 1: array<string, mixed>, 2?: array<string, mixed>}> */
41+
/** @return array<string, array{0: Result, 1: array<int|string, mixed>, 2?: array<int|string, mixed>}> */
4142
public static function provideForArray(): array
4243
{
4344
return [
@@ -76,6 +77,65 @@ public static function provideForArray(): array
7677
'3rd' => '__3rd_original__',
7778
],
7879
],
80+
'with string key collision' => [
81+
(new ResultBuilder())->id('root')->template('root_msg')
82+
->children(
83+
(new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path('foo'))->build(),
84+
(new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path('foo'))->build(),
85+
)->build(),
86+
[
87+
'foo' => ['msg1', 'msg2'],
88+
],
89+
],
90+
'with numeric key collision (list)' => [
91+
(new ResultBuilder())->id('root')->template('root_msg')
92+
->children(
93+
(new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path(0))->build(),
94+
(new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path(0))->build(),
95+
)->build(),
96+
[
97+
'__root__' => 'root_msg',
98+
0 => 'msg1',
99+
1 => 'msg2',
100+
],
101+
],
102+
'with mixed keys replacement' => [
103+
(new ResultBuilder())->id('root')->template('root_msg')
104+
->children(
105+
(new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path('foo'))->build(),
106+
(new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path(0))->build(),
107+
)->build(),
108+
[
109+
'__root__' => 'root_msg',
110+
'foo' => 'msg1',
111+
'c2' => 'msg2',
112+
],
113+
],
114+
'with mixed keys and ID collision' => [
115+
(new ResultBuilder())->id('root')->template('root_msg')
116+
->children(
117+
(new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path('foo'))->build(),
118+
(new ResultBuilder())->id('sameId')->template('msg2')->withPath(new Path(0))->build(),
119+
(new ResultBuilder())->id('sameId')->template('msg3')->withPath(new Path(1))->build(),
120+
)->build(),
121+
[
122+
'__root__' => 'root_msg',
123+
'foo' => 'msg1',
124+
'sameId' => ['msg2', 'msg3'],
125+
],
126+
],
127+
'with pure numeric keys' => [
128+
(new ResultBuilder())->id('root')->template('root_msg')
129+
->children(
130+
(new ResultBuilder())->id('c1')->template('msg1')->withPath(new Path(10))->build(),
131+
(new ResultBuilder())->id('c2')->template('msg2')->withPath(new Path(20))->build(),
132+
)->build(),
133+
[
134+
'__root__' => 'root_msg',
135+
0 => 'msg1',
136+
1 => 'msg2',
137+
],
138+
],
79139
];
80140
}
81141
}

0 commit comments

Comments
 (0)