Handle siblings when creating messages

The way we display messages could have been better, and it took me a while to realise that to make it better, I would need to handle the siblings of a result before deciding whether we should render it. Another issue was that rules like Key and Property had to create a "dumb" parent just so we would display the messages correctly, and in some cases, that wasn't even enough. This commit introduces quite a few changes to how the library works, making the messages much more straightforward.
Respect · Dec 13, 2024 · 82cb05b · 82cb05b
1 parent 52e628f
commit 82cb05b
Show file tree

Hide file tree

Showing 15 changed files with 193 additions and 72 deletions.
diff --git a/library/Message/Formatter.php b/library/Message/Formatter.php
@@ -21,7 +21,13 @@ public function main(Result $result, array $templates, Translator $translator):
     /**
      * @param array<string, mixed> $templates
      */
-    public function full(Result $result, array $templates, Translator $translator, int $depth = 0): string;
+    public function full(
+        Result $result,
+        array $templates,
+        Translator $translator,
+        int $depth = 0,
+        Result ...$siblings
+    ): string;
 
     /**
      * @param array<string, mixed> $templates

diff --git a/library/Message/StandardFormatter.php b/library/Message/StandardFormatter.php
@@ -14,6 +14,7 @@
 
 use function array_filter;
 use function array_key_exists;
+use function array_reduce;
 use function array_values;
 use function count;
 use function current;
@@ -51,13 +52,18 @@ public function main(Result $result, array $templates, Translator $translator):
     /**
      * @param array<string, mixed> $templates
      */
-    public function full(Result $result, array $templates, Translator $translator, int $depth = 0): string
-    {
+    public function full(
+        Result $result,
+        array $templates,
+        Translator $translator,
+        int $depth = 0,
+        Result ...$siblings
+    ): string {
         $selectedTemplates = $this->selectTemplates($result, $templates);
         $isFinalTemplate = $this->isFinalTemplate($result, $selectedTemplates);
 
         $rendered = '';
-        if ($result->isAlwaysVisible() || $isFinalTemplate) {
+        if ($this->isAlwaysVisible($result, ...$siblings) || $isFinalTemplate) {
             $indentation = str_repeat(' ', $depth * 2);
             $rendered .= sprintf(
                 '%s- %s' . PHP_EOL,
@@ -68,8 +74,15 @@ public function full(Result $result, array $templates, Translator $translator, i
         }
 
         if (!$isFinalTemplate) {
-            foreach ($this->extractDeduplicatedChildren($result) as $child) {
-                $rendered .= $this->full($child, $selectedTemplates, $translator, $depth);
+            $results = $this->extractDeduplicatedChildren($result);
+            foreach ($results as $child) {
+                $rendered .= $this->full(
+                    $child,
+                    $selectedTemplates,
+                    $translator,
+                    $depth,
+                    ...array_filter($results, static fn (Result $sibling) => $sibling !== $child)
+                );
                 $rendered .= PHP_EOL;
             }
         }
@@ -117,6 +130,41 @@ public function array(Result $result, array $templates, Translator $translator):
         return $messages;
     }
 
+    private function isAlwaysVisible(Result $result, Result ...$siblings): bool
+    {
+        if ($result->isValid) {
+            return false;
+        }
+
+        if ($result->hasCustomTemplate()) {
+            return true;
+        }
+
+        $childrenAlwaysVisible = array_filter(
+            $result->children,
+            fn (Result $child) => $this->isAlwaysVisible($child, ...array_filter(
+                $result->children,
+                static fn (Result $sibling) => $sibling !== $child
+            ))
+        );
+        if (count($childrenAlwaysVisible) !== 1) {
+            return true;
+        }
+
+        if (count($siblings) === 0) {
+            return false;
+        }
+
+        return array_reduce(
+            $siblings,
+            fn (bool $carry, Result $currentSibling) => $carry || $this->isAlwaysVisible(
+                $currentSibling,
+                ...array_filter($siblings, static fn (Result $sibling) => $sibling !== $currentSibling)
+            ),
+            true
+        );
+    }
+
     /** @param array<string, mixed> $templates */
     private function getTemplated(Result $result, array $templates): Result
     {

diff --git a/library/Result.php b/library/Result.php
@@ -40,6 +40,7 @@ public function __construct(
         ?string $name = null,
         ?string $id = null,
         public readonly ?Result $subsequent = null,
+        public readonly bool $unchangeableId = false,
         Result ...$children,
     ) {
         $this->name = $rule->getName() ?? $name;
@@ -75,12 +76,21 @@ public function withTemplate(string $template): self
 
     public function withId(string $id): self
     {
+        if ($this->unchangeableId) {
+            return $this;
+        }
+
         return $this->clone(id: $id);
     }
 
+    public function withUnchangeableId(string $id): self
+    {
+        return $this->clone(id: $id, unchangeableId: true);
+    }
+
     public function withPrefixedId(string $prefix): self
     {
-        if ($this->id === $this->name) {
+        if ($this->id === $this->name || $this->unchangeableId) {
             return $this;
         }
 
@@ -142,21 +152,6 @@ public function hasCustomTemplate(): bool
         return preg_match('/__[0-9a-z_]+_/', $this->template) === 0;
     }
 
-    public function isAlwaysVisible(): bool
-    {
-        if ($this->isValid) {
-            return false;
-        }
-
-        if ($this->hasCustomTemplate()) {
-            return true;
-        }
-
-        $childrenAlwaysVisible = array_filter($this->children, static fn (Result $child) => $child->isAlwaysVisible());
-
-        return count($childrenAlwaysVisible) !== 1;
-    }
-
     public function allowsSubsequent(): bool
     {
         if ($this->children === [] && !$this->hasCustomTemplate()) {
@@ -182,6 +177,7 @@ private function clone(
         ?string $name = null,
         ?string $id = null,
         ?Result $subsequent = null,
+        ?bool $unchangeableId = null,
         ?array $children = null
     ): self {
         return new self(
@@ -194,6 +190,7 @@ private function clone(
             $name ?? $this->name,
             $id ?? $this->id,
             $subsequent ?? $this->subsequent,
+            $unchangeableId ?? $this->unchangeableId,
             ...($children ?? $this->children)
         );
     }

diff --git a/library/Rules/Key.php b/library/Rules/Key.php
@@ -37,13 +37,9 @@ public function evaluate(mixed $input): Result
             return $keyExistsResult;
         }
 
-        $child = $this->rule
+        return $this->rule
             ->evaluate($input[$this->key])
-            ->withId((string) $this->key);
-
-        return (new Result($child->isValid, $input, $this))
-            ->withChildren($child)
-            ->withId((string) $this->key)
+            ->withUnchangeableId((string) $this->key)
             ->withNameIfMissing($this->rule->getName() ?? (string) $this->key);
     }
 }
diff --git a/library/Rules/KeyOptional.php b/library/Rules/KeyOptional.php
@@ -37,13 +37,9 @@ public function evaluate(mixed $input): Result
             return $keyExistsResult->withInvertedMode();
         }
 
-        $child = $this->rule
+        return $this->rule
             ->evaluate($input[$this->key])
-            ->withId((string) $this->key);
-
-        return (new Result($child->isValid, $input, $this))
-            ->withChildren($child)
-            ->withId((string) $this->key)
+            ->withUnchangeableId((string) $this->key)
             ->withNameIfMissing($this->rule->getName() ?? (string) $this->key);
     }
 }
diff --git a/library/Rules/Property.php b/library/Rules/Property.php
@@ -34,13 +34,9 @@ public function evaluate(mixed $input): Result
             return $propertyExistsResult;
         }
 
-        $childResult = $this->rule
+        return $this->rule
             ->evaluate($this->extractPropertyValue($input, $this->propertyName))
-            ->withId($this->propertyName);
-
-        return (new Result($childResult->isValid, $input, $this))
-            ->withChildren($childResult)
-            ->withId($this->propertyName)
+            ->withUnchangeableId($this->propertyName)
             ->withNameIfMissing($this->rule->getName() ?? $this->propertyName);
     }
 }
diff --git a/library/Rules/PropertyOptional.php b/library/Rules/PropertyOptional.php
@@ -34,13 +34,9 @@ public function evaluate(mixed $input): Result
             return $propertyExistsResult->withInvertedMode();
         }
 
-        $childResult = $this->rule
+        return $this->rule
             ->evaluate($this->extractPropertyValue($input, $this->propertyName))
-            ->withId($this->propertyName);
-
-        return (new Result($childResult->isValid, $input, $this))
-            ->withChildren($childResult)
-            ->withId($this->propertyName)
+            ->withUnchangeableId($this->propertyName)
             ->withNameIfMissing($this->rule->getName() ?? $this->propertyName);
     }
 }
diff --git a/tests/integration/issues/179.phpt b/tests/integration/issues/179.phpt
@@ -22,8 +22,9 @@ exceptionAll('https://github.com/Respect/Validation/issues/179', static fn() =>
 https://github.com/Respect/Validation/issues/179
 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
 host must be a string
-- host must be a string
-- user must be present
+- These rules must pass for Settings
+  - host must be a string
+  - user must be present
 [
     '__root__' => 'These rules must pass for Settings',
     'host' => 'host must be a string',

diff --git a/tests/integration/issues/446.phpt b/tests/integration/issues/446.phpt
@@ -19,8 +19,7 @@ exceptionAll('https://github.com/Respect/Validation/issues/446', static function
 https://github.com/Respect/Validation/issues/446
 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
 The length of name must be between 2 and 32
-- These rules must pass for `["name": "w", "email": "[email protected]"]`
-  - The length of name must be between 2 and 32
+- The length of name must be between 2 and 32
 [
     'name' => 'The length of name must be between 2 and 32',
 ]
diff --git a/tests/integration/issues/799.phpt b/tests/integration/issues/799.phpt
@@ -43,8 +43,7 @@ https://github.com/Respect/Validation/issues/799
 https://github.com/Respect/Validation/issues/799
 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
 scheme must start with "https"
-- These rules must pass for `["scheme": "http", "host": "www.google.com", "path": "/search", "query": "q=respect.github.com"]`
-  - scheme must start with "https"
+- scheme must start with "https"
 [
     'scheme' => 'scheme must start with "https"',
 ]
diff --git a/tests/integration/issues/805.phpt b/tests/integration/issues/805.phpt
@@ -13,7 +13,6 @@ https://github.com/Respect/Validation/issues/805
 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
 WRONG EMAIL!!!!!!
 - WRONG EMAIL!!!!!!
-  - WRONG EMAIL!!!!!!
 [
     'email' => 'WRONG EMAIL!!!!!!',
 ]
diff --git a/tests/integration/rules/keySet.phpt b/tests/integration/rules/keySet.phpt
@@ -193,8 +193,9 @@ foo must be present
 multiple rules / two extra keys
 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
 qux must be an integer
-- qux must be an integer
-- baz must not be present
+- `["foo": 42, "bar": "string", "baz": true, "qux": false]` contains extra keys
+  - qux must be an integer
+  - baz must not be present
 [
     '__root__' => '`["foo": 42, "bar": "string", "baz": true, "qux": false]` contains extra keys',
     'qux' => 'qux must be an integer',
@@ -216,8 +217,9 @@ bar must be an integer
 multiple rules / single missing key / single failed validation
 ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
 bar must be an integer
-- bar must be an integer
-- baz must be present
+- `["foo": 42, "bar": "string"]` contains missing keys
+  - bar must be an integer
+  - baz must be present
 [
     '__root__' => '`["foo": 42, "bar": "string"]` contains missing keys',
     'bar' => 'bar must be an integer',

diff --git a/tests/library/Builders/ResultBuilder.php b/tests/library/Builders/ResultBuilder.php
@@ -35,6 +35,8 @@ final class ResultBuilder
 
     private ?Result $subsequent = null;
 
+    private bool $unchangeableId = false;
+
     /** @var array<Result> */
     private array $children = [];
 
@@ -55,22 +57,14 @@ public function build(): Result
             $this->name,
             $this->id,
             $this->subsequent,
+            $this->unchangeableId,
             ...$this->children
         );
     }
 
-    public function isAlwaysVisible(): self
+    public function isValid(bool $isValid): self
     {
-        return $this->withCustomTemplate();
-    }
-
-    public function isNotAlwaysVisible(): self
-    {
-        $this->template = 'Custom template';
-        $this->children = [
-            (new self())->withCustomTemplate()->build(),
-            (new self())->children((new self())->withCustomTemplate()->build())->build(),
-        ];
+        $this->isValid = $isValid;
 
         return $this;
     }