Skip to content

Commit

Permalink
Use paths to identify when a rule fails
Browse files Browse the repository at this point in the history
When nested-structural validation fails, it's challenging to identify
which rule failed from the main exception message. A great example is
the `Issue796Test.php` file. The exception message says:

host must be a string

But you're left unsure whether it's the `host` key from the `mysql` key
or the `postgresql` key.

This commit changes that behaviour by introducing the concept of "Path."
The `path` represents the path that a rule has taken, and we can use it
in structural rules to identify the path of an array or object.

Here's what it looks like before and after:

```diff
-host must be a string
+`.mysql.host` must be a string
```

Because paths are a specific concept, I added a dot (`.`) at the
beginning of all paths when displaying them. I was inspired by the `jq`
syntax. I also added backticks around paths to distinguish them from any
other value.

I didn't manage to fix a test, and I skipped it instead of fixing it
because I want to make changes in how we display error messages as
arrays, and it will be easier to fix it then.
  • Loading branch information
henriquemoody committed Dec 27, 2024
1 parent a0d6355 commit 1915b6f
Show file tree
Hide file tree
Showing 46 changed files with 632 additions and 562 deletions.
5 changes: 5 additions & 0 deletions library/Message/Placeholder/Quoted.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ public function __construct(
) {
}

public static function fromPath(int|string $path): self
{
return new self('.' . $path);
}

public function getValue(): string
{
return $this->value;
Expand Down
117 changes: 80 additions & 37 deletions library/Message/StandardFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use function array_filter;
use function array_key_exists;
use function array_map;
use function array_reduce;
use function array_values;
use function count;
Expand All @@ -35,22 +36,22 @@ public function __construct(
}

/**
* @param array<string, mixed> $templates
* @param array<string|int, mixed> $templates
*/
public function main(Result $result, array $templates, Translator $translator): string
{
$selectedTemplates = $this->selectTemplates($result, $templates);
if (!$this->isFinalTemplate($result, $selectedTemplates)) {
foreach ($this->extractDeduplicatedChildren($result) as $child) {
return $this->main($child, $selectedTemplates, $translator);
return $this->main($this->resultWithPath($result, $child), $selectedTemplates, $translator);
}
}

return $this->renderer->render($this->getTemplated($result, $selectedTemplates), $translator);
}

/**
* @param array<string, mixed> $templates
* @param array<string|int, mixed> $templates
*/
public function full(
Result $result,
Expand All @@ -68,13 +69,19 @@ public function full(
$rendered .= sprintf(
'%s- %s' . PHP_EOL,
$indentation,
$this->renderer->render($this->getTemplated($result, $selectedTemplates), $translator),
$this->renderer->render(
$this->getTemplated($depth > 0 ? $result->withDeepestPath() : $result, $selectedTemplates),
$translator
),
);
$depth++;
}

if (!$isFinalTemplate) {
$results = $this->extractDeduplicatedChildren($result);
$results = array_map(
fn(Result $child) => $this->resultWithPath($result, $child),
$this->extractDeduplicatedChildren($result)
);
foreach ($results as $child) {
$rendered .= $this->full(
$child,
Expand All @@ -91,37 +98,44 @@ public function full(
}

/**
* @param array<string, mixed> $templates
* @param array<string|int, mixed> $templates
*
* @return array<string, mixed>
* @return array<string|int, mixed>
*/
public function array(Result $result, array $templates, Translator $translator): array
{
$selectedTemplates = $this->selectTemplates($result, $templates);
$deduplicatedChildren = $this->extractDeduplicatedChildren($result);
if (count($deduplicatedChildren) === 0 || $this->isFinalTemplate($result, $selectedTemplates)) {
return [
$result->id => $this->renderer->render($this->getTemplated($result, $selectedTemplates), $translator),
$result->getDeepestPath() ?? $result->id => $this->renderer->render(
$this->getTemplated($result->withDeepestPath(), $selectedTemplates),
$translator
),
];
}

$messages = [];
foreach ($deduplicatedChildren as $child) {
$messages[$child->id] = $this->array(
$child,
$key = $child->getDeepestPath() ?? $child->id;
$messages[$key] = $this->array(
$this->resultWithPath($result, $child),
$this->selectTemplates($child, $selectedTemplates),
$translator
);
if (count($messages[$child->id]) !== 1) {
if (count($messages[$key]) !== 1) {
continue;
}

$messages[$child->id] = current($messages[$child->id]);
$messages[$key] = current($messages[$key]);
}

if (count($messages) > 1) {
$self = [
'__root__' => $this->renderer->render($this->getTemplated($result, $selectedTemplates), $translator),
'__root__' => $this->renderer->render(
$this->getTemplated($result->withDeepestPath(), $selectedTemplates),
$translator
),
];

return $self + $messages;
Expand All @@ -130,6 +144,19 @@ public function array(Result $result, array $templates, Translator $translator):
return $messages;
}

public function resultWithPath(Result $parent, Result $child): Result
{
if ($parent->path !== null && $child->path !== null && $child->path !== $parent->path) {
return $child->withPath($parent->path);
}

if ($parent->path !== null && $child->path === null) {
return $child->withPath($parent->path);
}

return $child;
}

private function isAlwaysVisible(Result $result, Result ...$siblings): bool
{
if ($result->isValid) {
Expand Down Expand Up @@ -165,56 +192,66 @@ private function isAlwaysVisible(Result $result, Result ...$siblings): bool
);
}

/** @param array<string, mixed> $templates */
/** @param array<string|int, mixed> $templates */
private function getTemplated(Result $result, array $templates): Result
{
if ($result->hasCustomTemplate()) {
return $result;
}

if (!isset($templates[$result->id]) && isset($templates['__root__'])) {
return $result->withTemplate($templates['__root__']);
}
foreach ([$result->path, $result->name, $result->id, '__root__'] as $key) {
if (!isset($templates[$key])) {
continue;
}

if (!isset($templates[$result->id])) {
return $result;
}
if (is_string($templates[$key])) {
return $result->withTemplate($templates[$key]);
}

$template = $templates[$result->id];
if (is_string($template)) {
return $result->withTemplate($template);
throw new ComponentException(
sprintf('Template for "%s" must be a string, %s given', $key, stringify($templates[$key]))
);
}

throw new ComponentException(
sprintf('Template for "%s" must be a string, %s given', $result->id, stringify($template))
);
return $result;
}

/**
* @param array<string, mixed> $templates
* @param array<string|int, mixed> $templates
*/
private function isFinalTemplate(Result $result, array $templates): bool
{
if (isset($templates[$result->id]) && is_string($templates[$result->id])) {
return true;
$keys = [$result->path, $result->name, $result->id];
foreach ($keys as $key) {
if (isset($templates[$key]) && is_string($templates[$key])) {
return true;
}
}

if (count($templates) !== 1) {
return false;
}

return isset($templates['__root__']) || isset($templates[$result->id]);
foreach ($keys as $key) {
if (isset($templates[$key])) {
return true;
}
}

return isset($templates['__root__']);
}

/**
* @param array<string, mixed> $templates
* @param array<string|int, mixed> $templates
*
* @return array<string, mixed>
* @return array<string|int, mixed>
*/
private function selectTemplates(Result $message, array $templates): array
private function selectTemplates(Result $result, array $templates): array
{
if (isset($templates[$message->id]) && is_array($templates[$message->id])) {
return $templates[$message->id];
foreach ([$result->path, $result->name, $result->id] as $key) {
if (isset($templates[$key]) && is_array($templates[$key])) {
return $templates[$key];
}
}

return $templates;
Expand All @@ -227,7 +264,7 @@ private function extractDeduplicatedChildren(Result $result): array
$deduplicatedResults = [];
$duplicateCounters = [];
foreach ($result->children as $child) {
$id = $child->id;
$id = $child->getDeepestPath() ?? $child->id;
if (isset($duplicateCounters[$id])) {
$id .= '.' . ++$duplicateCounters[$id];
} elseif (array_key_exists($id, $deduplicatedResults)) {
Expand All @@ -236,7 +273,13 @@ private function extractDeduplicatedChildren(Result $result): array
$duplicateCounters[$id] = 2;
$id .= '.2';
}
$deduplicatedResults[$id] = $child->isValid ? null : $child->withId($id);

if ($child->path === null) {
$deduplicatedResults[$id] = $child->isValid ? null : $child->withId($id);
continue;
}

$deduplicatedResults[$id] = $child->isValid ? null : $child;
}

return array_values(array_filter($deduplicatedResults));
Expand Down
5 changes: 4 additions & 1 deletion library/Message/StandardRenderer.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ public function __construct(
public function render(Result $result, Translator $translator, ?string $template = null): string
{
$parameters = $result->parameters;
$parameters['name'] ??= $result->name ?? $this->placeholder('input', $result->input, $translator);
$parameters['path'] = $result->path !== null ? Quoted::fromPath($result->path) : null;
$parameters['input'] = $result->input;

$builtName = $result->name ?? $parameters['path'] ?? $this->placeholder('input', $result->input, $translator);
$parameters['name'] ??= $builtName;

$rendered = (string) preg_replace_callback(
'/{{(\w+)(\|([^}]+))?}}/',
function (array $matches) use ($parameters, $translator) {
Expand Down
53 changes: 40 additions & 13 deletions library/Result.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@
namespace Respect\Validation;

use Respect\Validation\Rules\Core\Nameable;
use Respect\Validation\Rules\Core\Renameable;

use function array_filter;
use function array_map;
use function count;
use function end;
use function explode;
use function lcfirst;
use function preg_match;
use function strrchr;
Expand All @@ -39,7 +40,7 @@ public function __construct(
public readonly ?string $name = null,
?string $id = null,
public readonly ?Result $adjacent = null,
public readonly bool $unchangeableId = false,
public readonly string|int|null $path = null,
Result ...$children,
) {
$this->id = $id ?? lcfirst(substr((string) strrchr($rule::class, '\\'), 1));
Expand Down Expand Up @@ -102,10 +103,6 @@ public function withExtraParameters(array $parameters): self

public function withId(string $id): self
{
if ($this->unchangeableId) {
return $this;
}

return $this->clone(id: $id);
}

Expand All @@ -114,14 +111,44 @@ public function withIdFrom(Rule $rule): self
return $this->clone(id: lcfirst(substr((string) strrchr($rule::class, '\\'), 1)));
}

public function withUnchangeableId(string $id): self
public function withPath(string|int $path): self
{
return $this->clone(
adjacent: $this->adjacent?->withPath($path),
path: $this->path === null ? $path : $path . '.' . $this->path,
);
}

public function withDeepestPath(): self
{
$path = $this->getDeepestPath();
if ($path === null || $path === (string) $this->path) {
return $this;
}

return $this->clone(
adjacent: $this->adjacent?->withPath($path),
path: $path,
);
}

public function getDeepestPath(): ?string
{
return $this->clone(id: $id, unchangeableId: true);
if ($this->path === null) {
return null;
}

$paths = explode('.', (string) $this->path);
if (count($paths) === 1) {
return (string) $this->path;
}

return end($paths);
}

public function withPrefix(string $prefix): self
{
if ($this->id === $this->name || $this->unchangeableId) {
if ($this->id === $this->name || $this->path !== null) {
return $this;
}

Expand All @@ -136,10 +163,10 @@ public function withChildren(Result ...$children): self
public function withName(string $name): self
{
return $this->clone(
name: $this->rule instanceof Renameable ? $name : ($this->name ?? $name),
name: $this->name ?? $name,
adjacent: $this->adjacent?->withName($name),
children: array_map(
static fn (Result $child) => $child->withName($child->name ?? $name),
static fn (Result $child) => $child->path === null ? $child->withName($child->name ?? $name) : $child,
$this->children
),
);
Expand Down Expand Up @@ -223,7 +250,7 @@ private function clone(
?string $name = null,
?string $id = null,
?Result $adjacent = null,
?bool $unchangeableId = null,
string|int|null $path = null,
?array $children = null
): self {
return new self(
Expand All @@ -236,7 +263,7 @@ private function clone(
$name ?? $this->name,
$id ?? $this->id,
$adjacent ?? $this->adjacent,
$unchangeableId ?? $this->unchangeableId,
$path ?? $this->path,
...($children ?? $this->children)
);
}
Expand Down
14 changes: 0 additions & 14 deletions library/Rules/Core/Renameable.php

This file was deleted.

Loading

0 comments on commit 1915b6f

Please sign in to comment.