Skip to content

Commit 8332d28

Browse files
henriquemoodyhenriquemoody
committed
Do not overwrite existing names
This commit addresses the skipped tests by modifying certain core concepts in the library. The way names work has always been somewhat confusing, even to me. I’ve established that existing names will never be overwritten, and if a path is already defined, we will change the name to also include the path. I’m not very happy with how I’m solving this problem, because the `FirstResultStringFormatter` is changing some behaviour in the results that seems to be something that should always happen before. But, for the moment, I will keep it as is.
1 parent 0d4a5fe commit 8332d28

18 files changed

+140
-89
lines changed

‎docs/rules/KeyExists.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,27 @@ v::keyExists(5)->isValid(new ArrayObject(['a', 'b', 'c'])); // false
3535
|-------------|------------------------------------------------------------------|
3636
| `name` | The validated input or the custom validator name (if specified). |
3737

38+
## Caveats
39+
40+
`KeyExists` defines the given `$key` as the path, and because it is a standalone rule without children, it's not possible to display a fully custom name with it.
41+
42+
When no custom name is set, the path is displayed as `{{name}}`. When a custom name is set, the validation engine prepends the path to the custom name:
43+
44+
```php
45+
v::keyExists('foo')->assert([]);
46+
// Message: `.foo` must be present
47+
48+
v::keyExists('foo')->setName('Custom name')->assert([]);
49+
// Message: `.foo` (<- Custom name) must be present
50+
```
51+
52+
If you want to display only a custom name while checking if a key exists, use [Key](Key.md) with [AlwaysValid](AlwaysValid.md):
53+
54+
```php
55+
v::key('foo', v::alwaysValid()->setName('Custom name'))->assert([]);
56+
// Message: Custom name must be present
57+
```
58+
3859
## Categorization
3960

4061
- Arrays

‎docs/rules/PropertyExists.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,27 @@ This rule will validate public, private, protected, uninitialised, and static pr
3636
|-------------|------------------------------------------------------------------|
3737
| `name` | The validated input or the custom validator name (if specified). |
3838

39+
## Caveats
40+
41+
`PropertyExists` defines the given `$propertyName` as the path, and because it is a standalone rule without children, it's not possible to display a fully custom name with it.
42+
43+
When no custom name is set, the path is displayed as `{{name}}`. When a custom name is set, the validation engine prepends the path to the custom name:
44+
45+
```php
46+
v::propertyExists('foo')->assert([]);
47+
// Message: `.foo` must be present
48+
49+
v::propertyExists('foo')->setName('Custom name')->assert([]);
50+
// Message: `.foo` (<- Custom name) must be present
51+
```
52+
53+
If you want to display only a custom name while checking if a property exists, use [Property](Property.md) with [AlwaysValid](AlwaysValid.md):
54+
55+
```php
56+
v::property('foo', v::alwaysValid()->setName('Custom name'))->assert([]);
57+
// Message: Custom name must be present
58+
```
59+
3960
## Categorization
4061

4162
- Objects

‎library/Message/Formatter/FirstResultStringFormatter.php‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,30 @@
1111

1212
use Respect\Validation\Message\Renderer;
1313
use Respect\Validation\Message\StringFormatter;
14+
use Respect\Validation\Name;
1415
use Respect\Validation\Result;
1516

1617
final readonly class FirstResultStringFormatter implements StringFormatter
1718
{
1819
/** @param array<string|int, mixed> $templates */
1920
public function format(Result $result, Renderer $renderer, array $templates): string
21+
{
22+
return $this->formatResult($result, $renderer, $templates, null);
23+
}
24+
25+
/** @param array<string|int, mixed> $templates */
26+
private function formatResult(Result $result, Renderer $renderer, array $templates, Name|null $parentName): string
2027
{
2128
if (!$result->hasCustomTemplate()) {
2229
foreach ($result->children as $child) {
23-
return $this->format($child, $renderer, $templates);
30+
return $this->formatResult($child, $renderer, $templates, $result->name ?? $parentName);
2431
}
2532
}
2633

34+
if ($parentName !== null) {
35+
$result = $result->withName($parentName);
36+
}
37+
2738
return $renderer->render($result, $templates);
2839
}
2940
}

‎library/Message/InterpolationRenderer.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public function render(Result $result, array $templates): string
4545
$rendered = (string) preg_replace_callback(
4646
'/{{(\w+)(\|([^}]+))?}}/',
4747
function (array $matches) use ($parameters) {
48-
if (!isset($parameters[$matches[1]])) {
48+
if (!array_key_exists($matches[1], $parameters)) {
4949
return $matches[0];
5050
}
5151

@@ -89,24 +89,24 @@ private function placeholder(
8989
return $this->stringifier->stringify($value, 0) ?? print_r($value, true);
9090
}
9191

92-
private function getName(Result $result): Name
92+
private function getName(Result $result): mixed
9393
{
9494
if (array_key_exists('name', $result->parameters) && is_string($result->parameters['name'])) {
9595
return new Name($result->parameters['name']);
9696
}
9797

9898
if (array_key_exists('name', $result->parameters)) {
99-
return new Name((string) $this->stringifier->stringify($result->parameters['name'], 0));
99+
return $result->parameters['name'];
100100
}
101101

102102
if ($result->name !== null) {
103-
return $result->path ? $result->name->withPath($result->path) : $result->name;
103+
return $result->name;
104104
}
105105

106106
if ($result->path?->value !== null) {
107-
return new Name((string) $this->stringifier->stringify($result->path, 0));
107+
return $result->path;
108108
}
109109

110-
return new Name((string) $this->stringifier->stringify($result->input, 0));
110+
return $result->input;
111111
}
112112
}

‎library/Message/Stringifier/NameStringifier.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ public function stringify(mixed $raw, int $depth): string|null
2727
return null;
2828
}
2929

30-
if ($raw->path === null || $raw->path->isOrphan()) {
30+
if ($raw->path === null) {
3131
return $raw->value;
3232
}
3333

‎library/Path.php‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,24 +12,8 @@
1212
final class Path
1313
{
1414
public function __construct(
15-
public int|string $value,
15+
public readonly int|string $value,
1616
public Path|null $parent = null,
1717
) {
1818
}
19-
20-
public function isOrphan(): bool
21-
{
22-
return $this->parent === null;
23-
}
24-
25-
public function withParent(self $parent): self
26-
{
27-
if ($parent === $this->parent) {
28-
return $this;
29-
}
30-
31-
$this->parent = $parent;
32-
33-
return $this;
34-
}
3519
}

‎library/Result.php‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ public function withPath(Path $path): self
118118
}
119119

120120
if ($this->path !== null) {
121-
$this->path->withParent($path);
121+
$this->path->parent = $path;
122122

123123
return $this;
124124
}
@@ -160,11 +160,15 @@ public function withChildren(Result ...$children): self
160160

161161
public function withName(Name $name): self
162162
{
163+
if ($this->path !== null && $this->name?->path !== $this->path) {
164+
$name = $name->withPath($this->path);
165+
}
166+
163167
return clone($this, [
164168
'name' => $this->name ?? $name,
165169
'adjacent' => $this->adjacent?->withName($name),
166170
'children' => array_map(
167-
static fn(Result $child) => $child->withName($child->name ?? $name),
171+
static fn(Result $child) => $child->path === null ? $child->withName($child->name ?? $name) : $child,
168172
$this->children,
169173
),
170174
]);
@@ -173,7 +177,14 @@ public function withName(Name $name): self
173177
public function withNameFrom(Rule $rule): self
174178
{
175179
if ($rule instanceof Nameable && $rule->getName() !== null) {
176-
return $this->withName($rule->getName());
180+
return clone($this, [
181+
'name' => $this->name ?? $rule->getName(),
182+
'adjacent' => $this->adjacent?->withNameFrom($rule),
183+
'children' => array_map(
184+
static fn(Result $child) => $child->withNameFrom($rule),
185+
$this->children,
186+
),
187+
]);
177188
}
178189

179190
return $this;

‎phpstan.neon.dist‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ parameters:
77
# Why: SimpleXMLElement is weird and doesn't implement anything ArrayAccess-like
88
message: '/Instanceof between mixed and SimpleXMLElement will always evaluate to false\./'
99
path: library/Rules/ArrayVal.php
10-
- message: '/Call to an undefined method .+::(skip|throws)\(\)/'
11-
path: tests/feature
1210
- message: '/Call to an undefined method .+::expectException\(\)/'
1311
path: tests/Pest.php
1412
- message: '/Undefined variable: \$this/'

tests/feature/ShouldNotOverwriteDefinedNamesTest.php renamed to tests/feature/HandlingNamesTest.php

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,22 @@
99

1010
$input = ['email' => 'not an email'];
1111

12-
test('Scenario #1', catchMessage(
12+
test('Results with a defined name cannot be overwritten by another name', catchMessage(
1313
fn() => v::key('email', v::email()->setName('Email'))->setName('Foo')->assert($input),
1414
fn(string $message) => expect($message)->toBe('Email must be a valid email address'),
1515
));
1616

17-
test('Scenario #2', catchMessage(
17+
test('Defining a name to a result with a path will add the path to the name', catchMessage(
1818
fn() => v::key('email', v::email())->setName('Email')->assert($input),
19+
fn(string $message) => expect($message)->toBe('`.email` (<- Email) must be a valid email address'),
20+
));
21+
22+
test('Results with a defined name will not be affected by a path', catchMessage(
23+
fn() => v::key('email', v::email()->setName('Email'))->assert($input),
1924
fn(string $message) => expect($message)->toBe('Email must be a valid email address'),
2025
));
2126

22-
test('Scenario #3', catchMessage(
27+
test('Not defining a name to a result with a path will display the path', catchMessage(
2328
fn() => v::key('email', v::email())->assert($input),
2429
fn(string $message) => expect($message)->toBe('`.email` must be a valid email address'),
2530
));

‎tests/feature/Issues/Issue179Test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,4 @@ function (): void {
3535
'host' => '`.host` must be a string',
3636
'user' => '`.user` must be present',
3737
]),
38-
))->skip('This still needs to be fixed in order to pass.');
38+
));

0 commit comments

Comments
 (0)