From ec70cf7bc2bdab9b39df8e0a6a028fb36a3301cc Mon Sep 17 00:00:00 2001 From: Henrique Moody Date: Tue, 30 Dec 2025 11:32:56 +0100 Subject: [PATCH] Ensure formatters only format, not modify results When I changed the library to not overwrite existing names [1], I wasn't happy with how `FirstResultStringFormatter` was changing the results, because the results should be completely ready by the time they arrive in the formatters. This commit changes that behaviour, ensuring the results are complete with all necessary information by the time they reach the formatters. Along with those changes, I refactored some stringifiers and simplified the `InterpolationRenderer`; we were not overwriting the "name" parameter anyway, as it was just an unnecessary overhead. [1] 8332d28acce8c132331baaabc220215a76c6a28c --- .../Formatter/FirstResultStringFormatter.php | 13 +- .../Formatter/NestedListStringFormatter.php | 19 ++- library/Message/InterpolationRenderer.php | 25 +-- library/Message/Placeholder/Subject.php | 30 ++++ .../Message/Stringifier/NameStringifier.php | 17 +- .../Stringifier/SubjectStringifier.php | 48 ++++++ library/Message/ValidationStringifier.php | 4 +- library/Name.php | 5 - library/Result.php | 37 +++-- library/Rules/Key.php | 2 +- library/Rules/Property.php | 2 +- tests/library/Builders/ResultBuilder.php | 3 + .../Message/InterpolationRendererTest.php | 63 ++------ .../Stringifier/SubjectStringifierTest.php | 146 ++++++++++++++++++ 14 files changed, 284 insertions(+), 130 deletions(-) create mode 100644 library/Message/Placeholder/Subject.php create mode 100644 library/Message/Stringifier/SubjectStringifier.php create mode 100644 tests/unit/Message/Stringifier/SubjectStringifierTest.php diff --git a/library/Message/Formatter/FirstResultStringFormatter.php b/library/Message/Formatter/FirstResultStringFormatter.php index 9426d929a..52e673907 100644 --- a/library/Message/Formatter/FirstResultStringFormatter.php +++ b/library/Message/Formatter/FirstResultStringFormatter.php @@ -11,30 +11,19 @@ use Respect\Validation\Message\Renderer; use Respect\Validation\Message\StringFormatter; -use Respect\Validation\Name; use Respect\Validation\Result; final readonly class FirstResultStringFormatter implements StringFormatter { /** @param array $templates */ public function format(Result $result, Renderer $renderer, array $templates): string - { - return $this->formatResult($result, $renderer, $templates, null); - } - - /** @param array $templates */ - private function formatResult(Result $result, Renderer $renderer, array $templates, Name|null $parentName): string { if (!$result->hasCustomTemplate()) { foreach ($result->children as $child) { - return $this->formatResult($child, $renderer, $templates, $result->name ?? $parentName); + return $this->format($child, $renderer, $templates); } } - if ($parentName !== null) { - $result = $result->withName($parentName); - } - return $renderer->render($result, $templates); } } diff --git a/library/Message/Formatter/NestedListStringFormatter.php b/library/Message/Formatter/NestedListStringFormatter.php index 034236332..35a8ed774 100644 --- a/library/Message/Formatter/NestedListStringFormatter.php +++ b/library/Message/Formatter/NestedListStringFormatter.php @@ -11,6 +11,7 @@ use Respect\Validation\Message\Renderer; use Respect\Validation\Message\StringFormatter; +use Respect\Validation\Name; use Respect\Validation\Result; use function array_filter; @@ -27,7 +28,7 @@ /** @param array $templates */ public function format(Result $result, Renderer $renderer, array $templates): string { - return $this->formatRecursively($result, $renderer, $templates, 0); + return $this->formatRecursively($result, $renderer, $templates, 0, null); } /** @param array $templates */ @@ -36,23 +37,31 @@ private function formatRecursively( Renderer $renderer, array $templates, int $depth, + Name|null $lastVisibleName, Result ...$siblings, ): string { $formatted = ''; - $displayedName = null; if ($this->isVisible($result, ...$siblings)) { $indentation = str_repeat(' ', $depth * 2); - $displayedName = $result->name; - $formatted .= sprintf('%s- %s' . PHP_EOL, $indentation, $renderer->render($result, $templates)); + $formatted .= sprintf( + '%s- %s' . PHP_EOL, + $indentation, + $renderer->render( + $lastVisibleName === $result->name ? $result->withoutName() : $result, + $templates, + ), + ); + $lastVisibleName ??= $result->name; $depth++; } foreach ($result->children as $child) { $formatted .= $this->formatRecursively( - $displayedName === $child->name ? $child->withoutName() : $child, + $child, $renderer, $templates, $depth, + $lastVisibleName, ...array_filter($result->children, static fn(Result $sibling) => $sibling !== $child), ); $formatted .= PHP_EOL; diff --git a/library/Message/InterpolationRenderer.php b/library/Message/InterpolationRenderer.php index 71e9c015e..0ebc749a7 100644 --- a/library/Message/InterpolationRenderer.php +++ b/library/Message/InterpolationRenderer.php @@ -10,7 +10,7 @@ namespace Respect\Validation\Message; use Respect\Validation\Message\Formatter\TemplateResolver; -use Respect\Validation\Name; +use Respect\Validation\Message\Placeholder\Subject; use Respect\Validation\Result; use function array_key_exists; @@ -31,7 +31,7 @@ public function __construct( /** @param array $templates */ public function render(Result $result, array $templates): string { - $parameters = ['path' => $result->path, 'input' => $result->input, 'subject' => $this->getName($result)]; + $parameters = ['path' => $result->path, 'input' => $result->input, 'subject' => Subject::fromResult($result)]; $parameters += $result->parameters; $givenTemplate = $this->templateResolver->getGivenTemplate($result, $templates); @@ -65,25 +65,4 @@ private function processPlaceholder(array $parameters, array $matches): string return $this->modifier->modify($parameters[$name], $pipe); } - - private function getName(Result $result): mixed - { - if (array_key_exists('name', $result->parameters) && is_string($result->parameters['name'])) { - return new Name($result->parameters['name']); - } - - if (array_key_exists('name', $result->parameters)) { - return $result->parameters['name']; - } - - if ($result->name !== null) { - return $result->name; - } - - if ($result->path?->value !== null) { - return $result->path; - } - - return $result->input; - } } diff --git a/library/Message/Placeholder/Subject.php b/library/Message/Placeholder/Subject.php new file mode 100644 index 000000000..bcf0d8b64 --- /dev/null +++ b/library/Message/Placeholder/Subject.php @@ -0,0 +1,30 @@ + + * SPDX-License-Identifier: MIT + */ + +declare(strict_types=1); + +namespace Respect\Validation\Message\Placeholder; + +use Respect\Validation\Name; +use Respect\Validation\Path; +use Respect\Validation\Result; + +final readonly class Subject +{ + public function __construct( + public mixed $input, + public Path|null $path = null, + public Name|null $name = null, + public bool $hasPrecedentName = true, + ) { + } + + public static function fromResult(Result $result): self + { + return new self($result->input, $result->path, $result->name, $result->hasPrecedentName); + } +} diff --git a/library/Message/Stringifier/NameStringifier.php b/library/Message/Stringifier/NameStringifier.php index f95689db9..d7fafeec9 100644 --- a/library/Message/Stringifier/NameStringifier.php +++ b/library/Message/Stringifier/NameStringifier.php @@ -12,29 +12,14 @@ use Respect\Stringifier\Stringifier; use Respect\Validation\Name; -use function sprintf; - final readonly class NameStringifier implements Stringifier { - public function __construct( - private Stringifier $stringifier, - ) { - } - public function stringify(mixed $raw, int $depth): string|null { if (!$raw instanceof Name) { return null; } - if ($raw->path === null) { - return $raw->value; - } - - return sprintf( - '%s (<- %s)', - $this->stringifier->stringify($raw->path, $depth), - $raw->value, - ); + return $raw->value; } } diff --git a/library/Message/Stringifier/SubjectStringifier.php b/library/Message/Stringifier/SubjectStringifier.php new file mode 100644 index 000000000..5a33b9330 --- /dev/null +++ b/library/Message/Stringifier/SubjectStringifier.php @@ -0,0 +1,48 @@ + + * SPDX-License-Identifier: MIT + */ + +declare(strict_types=1); + +namespace Respect\Validation\Message\Stringifier; + +use Respect\Stringifier\Stringifier; +use Respect\Validation\Message\Placeholder\Subject; + +use function sprintf; + +final readonly class SubjectStringifier implements Stringifier +{ + public function __construct( + private Stringifier $stringifier, + ) { + } + + public function stringify(mixed $raw, int $depth): string|null + { + if (!$raw instanceof Subject) { + return null; + } + + if ($raw->path === null && $raw->name === null) { + return $this->stringifier->stringify($raw->input, $depth); + } + + if ($raw->name === null) { + return $this->stringifier->stringify($raw->path, $depth); + } + + if ($raw->path === null || $raw->hasPrecedentName) { + return $this->stringifier->stringify($raw->name, $depth); + } + + return sprintf( + '%s (<- %s)', + $this->stringifier->stringify($raw->path, $depth), + $this->stringifier->stringify($raw->name, $depth), + ); + } +} diff --git a/library/Message/ValidationStringifier.php b/library/Message/ValidationStringifier.php index e217ebf71..a59d37941 100644 --- a/library/Message/ValidationStringifier.php +++ b/library/Message/ValidationStringifier.php @@ -35,6 +35,7 @@ use Respect\Validation\Message\Stringifier\NameStringifier; use Respect\Validation\Message\Stringifier\PathStringifier; use Respect\Validation\Message\Stringifier\QuotedStringifier; +use Respect\Validation\Message\Stringifier\SubjectStringifier; final readonly class ValidationStringifier implements Stringifier { @@ -95,7 +96,8 @@ private function createStringifier(Quoter $quoter): Stringifier $stringifier->prependStringifier(new PathStringifier($quoter)); $stringifier->prependStringifier(new QuotedStringifier($quoter)); $stringifier->prependStringifier(new ListedStringifier($stringifier)); - $stringifier->prependStringifier(new NameStringifier($stringifier)); + $stringifier->prependStringifier(new NameStringifier()); + $stringifier->prependStringifier(new SubjectStringifier($stringifier)); return $stringifier; } diff --git a/library/Name.php b/library/Name.php index 7be74097e..8f1c06fc1 100644 --- a/library/Name.php +++ b/library/Name.php @@ -16,9 +16,4 @@ public function __construct( public Path|null $path = null, ) { } - - public function withPath(Path $path): Name - { - return new self($this->value, $path); - } } diff --git a/library/Result.php b/library/Result.php index f8f01ec11..4e2fafaf3 100644 --- a/library/Result.php +++ b/library/Result.php @@ -30,6 +30,7 @@ public function __construct( public array $parameters = [], public string $template = Rule::TEMPLATE_STANDARD, public bool $hasInvertedMode = false, + public bool $hasPrecedentName = true, public Name|null $name = null, public Result|null $adjacent = null, public Path|null $path = null, @@ -124,6 +125,7 @@ public function withPath(Path $path): self return clone($this, [ 'path' => $path, 'adjacent' => $this->adjacent?->withPath($path), + 'hasPrecedentName' => $this->name !== null, 'children' => array_map( static fn(Result $child) => $child->withPath($path), $this->children, @@ -149,24 +151,20 @@ public function withoutName(): self public function withChildren(Result ...$children): self { - if ($this->path === null) { - return clone($this, ['children' => $children]); - } - - return clone($this, ['children' => array_map(fn(Result $child) => $child->withPath($this->path), $children)]); + return clone($this, ['children' => $children]); } public function withName(Name $name): self { - if ($this->path !== null && $this->name?->path !== $this->path) { - $name = $name->withPath($this->path); + if ($this->name !== null) { + return $this; } return clone($this, [ - 'name' => $this->name ?? $name, + 'name' => $name, 'adjacent' => $this->adjacent?->withName($name), 'children' => array_map( - static fn(Result $child) => $child->path === null ? $child->withName($child->name ?? $name) : $child, + static fn(Result $child) => $child->withName($name), $this->children, ), ]); @@ -174,18 +172,19 @@ public function withName(Name $name): self public function withNameFrom(Rule $rule): self { - if ($rule instanceof Nameable && $rule->getName() !== null) { - return clone($this, [ - 'name' => $this->name ?? $rule->getName(), - 'adjacent' => $this->adjacent?->withNameFrom($rule), - 'children' => array_map( - static fn(Result $child) => $child->withNameFrom($rule), - $this->children, - ), - ]); + if (!$rule instanceof Nameable || $rule->getName() === null) { + return $this; } - return $this; + return clone($this, [ + 'name' => $this->name ?? $rule->getName(), + 'hasPrecedentName' => true, + 'adjacent' => $this->adjacent?->withNameFrom($rule), + 'children' => array_map( + static fn(Result $child) => $child->withNameFrom($rule), + $this->children, + ), + ]); } public function withInput(mixed $input): self diff --git a/library/Rules/Key.php b/library/Rules/Key.php index 90f2a8806..10fa8ee89 100644 --- a/library/Rules/Key.php +++ b/library/Rules/Key.php @@ -38,6 +38,6 @@ public function evaluate(mixed $input): Result return $keyExistsResult->withNameFrom($this->rule); } - return $this->rule->evaluate($input[$this->key])->withPath(new Path($this->key)); + return $this->rule->evaluate($input[$this->key])->withPath($keyExistsResult->path ?? new Path($this->key)); } } diff --git a/library/Rules/Property.php b/library/Rules/Property.php index 82c7467da..242628039 100644 --- a/library/Rules/Property.php +++ b/library/Rules/Property.php @@ -36,7 +36,7 @@ public function evaluate(mixed $input): Result return $this->rule ->evaluate($this->getPropertyValue($input, $this->propertyName)) - ->withPath(new Path($this->propertyName)); + ->withPath($propertyExistsResult->path ?? new Path($this->propertyName)); } private function getPropertyValue(object $object, string $propertyName): mixed diff --git a/tests/library/Builders/ResultBuilder.php b/tests/library/Builders/ResultBuilder.php index 6d22f0e08..e1b65290a 100644 --- a/tests/library/Builders/ResultBuilder.php +++ b/tests/library/Builders/ResultBuilder.php @@ -24,6 +24,8 @@ final class ResultBuilder private bool $hasInvertedMode = false; + private bool $hasPrecedentName = true; + private string $template = Rule::TEMPLATE_STANDARD; /** @var array */ @@ -58,6 +60,7 @@ public function build(): Result $this->parameters, $this->template, $this->hasInvertedMode, + $this->hasPrecedentName, $this->name, $this->adjacent, $this->path, diff --git a/tests/unit/Message/InterpolationRendererTest.php b/tests/unit/Message/InterpolationRendererTest.php index 799d0d83a..a63e18b41 100644 --- a/tests/unit/Message/InterpolationRendererTest.php +++ b/tests/unit/Message/InterpolationRendererTest.php @@ -11,9 +11,9 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use Respect\Validation\Message\Placeholder\Subject; use Respect\Validation\Message\Translator\ArrayTranslator; use Respect\Validation\Message\Translator\DummyTranslator; -use Respect\Validation\Name; use Respect\Validation\Test\Builders\ResultBuilder; use Respect\Validation\Test\Message\TestingModifier; use Respect\Validation\Test\TestCase; @@ -73,47 +73,6 @@ public function itShouldRenderResultProcessingModifierParametersInTheTemplate(): ); } - #[Test] - public function itShouldRenderResultProcessingNameParameterWhenItIsInTheTemplateAndItIsString(): void - { - $modifier = new TestingModifier(); - $renderer = new InterpolationRenderer(new DummyTranslator(), $modifier); - - $value = 'original'; - - $result = (new ResultBuilder()) - ->template('Will replace {{subject}}') - ->parameters(['name' => $value]) - ->build(); - - self::assertSame( - sprintf('Will replace %s', $modifier->modify(new Name($value), null)), - $renderer->render($result, []), - ); - } - - #[Test] - public function itShouldRenderResultProcessingNameParameterWhenItIsInTheTemplateAndItIsNotString(): void - { - $modifier = new TestingModifier(); - $renderer = new InterpolationRenderer(new DummyTranslator(), $modifier); - - $value = true; - - $result = (new ResultBuilder()) - ->template('Will replace {{subject}}') - ->parameters(['name' => $value]) - ->build(); - - self::assertSame( - sprintf( - 'Will replace %s', - $modifier->modify($value, null), - ), - $renderer->render($result, []), - ); - } - #[Test] public function itShouldRenderResultProcessingNameAsSomeParameterInTheTemplate(): void { @@ -127,8 +86,10 @@ public function itShouldRenderResultProcessingNameAsSomeParameterInTheTemplate() ->name($name) ->build(); + $subject = Subject::fromResult($result); + self::assertSame( - 'Will replace ' . $modifier->modify(new Name($name), null), + 'Will replace ' . $modifier->modify($subject, null), $renderer->render($result, []), ); } @@ -146,10 +107,12 @@ public function itShouldRenderResultProcessingInputAsNameWhenResultHasNoName(): ->input($input) ->build(); + $subject = Subject::fromResult($result); + self::assertSame( sprintf( 'Will replace %s', - $modifier->modify($input, null), + $modifier->modify($subject, null), ), $renderer->render($result, []), ); @@ -188,8 +151,10 @@ public function itShouldRenderResultNotOverwritingNameParameterWithRealName(): v ->parameters(['name' => $parameterNameValue]) ->build(); + $subject = Subject::fromResult($result); + self::assertSame( - sprintf('Will replace %s', $modifier->modify(new Name($parameterNameValue), null)), + sprintf('Will replace %s', $modifier->modify($subject, null)), $renderer->render($result, []), ); } @@ -253,10 +218,12 @@ public function itShouldRenderResultWithNonCustomTemplate(): void $result = (new ResultBuilder())->build(); + $subject = Subject::fromResult($result); + self::assertSame( sprintf( '%s must be a valid stub', - $modifier->modify($result->input, null), + $modifier->modify($subject, null), ), $renderer->render($result, []), ); @@ -271,10 +238,12 @@ public function itShouldRenderResultWithNonCustomTemplateAndInvertedMode(): void $result = (new ResultBuilder())->hasInvertedMode()->build(); + $subject = Subject::fromResult($result); + self::assertSame( sprintf( '%s must not be a valid stub', - $modifier->modify($result->input, null), + $modifier->modify($subject, null), ), $renderer->render($result, []), ); diff --git a/tests/unit/Message/Stringifier/SubjectStringifierTest.php b/tests/unit/Message/Stringifier/SubjectStringifierTest.php new file mode 100644 index 000000000..cd0088f10 --- /dev/null +++ b/tests/unit/Message/Stringifier/SubjectStringifierTest.php @@ -0,0 +1,146 @@ + + * SPDX-License-Identifier: MIT + */ + +declare(strict_types=1); + +namespace Respect\Validation\Message\Stringifier; + +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Test; +use Respect\Validation\Message\Placeholder\Subject; +use Respect\Validation\Name; +use Respect\Validation\Path; +use Respect\Validation\Test\Message\TestingStringifier; +use Respect\Validation\Test\TestCase; +use stdClass; + +use function sprintf; + +#[CoversClass(SubjectStringifier::class)] +final class SubjectStringifierTest extends TestCase +{ + #[Test] + #[DataProvider('providerForNonSubjectValues')] + public function itShouldNotStringifyWhenValueIsNotAnInstanceOfSubject(mixed $value): void + { + $stringifier = new SubjectStringifier(new TestingStringifier()); + + self::assertNull($stringifier->stringify($value, 0)); + } + + #[Test] + public function itShouldStringifyInputWhenPathAndNameAreNull(): void + { + $input = ['test' => 'value']; + $subject = new Subject($input); + + $testingStringifier = new TestingStringifier(); + $stringifier = new SubjectStringifier($testingStringifier); + + $expected = $testingStringifier->stringify($input, 0); + $actual = $stringifier->stringify($subject, 0); + + self::assertSame($expected, $actual); + } + + #[Test] + public function itShouldStringifyPathWhenNameIsNull(): void + { + $path = new Path('field1'); + $subject = new Subject('input', $path); + + $testingStringifier = new TestingStringifier(); + $stringifier = new SubjectStringifier($testingStringifier); + + $expected = $testingStringifier->stringify($path, 0); + $actual = $stringifier->stringify($subject, 0); + + self::assertSame($expected, $actual); + } + + #[Test] + public function itShouldStringifyNameWhenPathIsNull(): void + { + $name = new Name('field_name'); + $subject = new Subject('input', null, $name); + + $testingStringifier = new TestingStringifier(); + $stringifier = new SubjectStringifier($testingStringifier); + + $expected = $testingStringifier->stringify($name, 0); + $actual = $stringifier->stringify($subject, 0); + + self::assertSame($expected, $actual); + } + + #[Test] + public function itShouldStringifyNameWhenNameHasPrecedence(): void + { + $path = new Path('field1'); + $name = new Name('field_name'); + $subject = new Subject('input', $path, $name, true); + + $testingStringifier = new TestingStringifier(); + $stringifier = new SubjectStringifier($testingStringifier); + + $expected = $testingStringifier->stringify($name, 0); + $actual = $stringifier->stringify($subject, 0); + + self::assertSame($expected, $actual); + } + + #[Test] + public function itShouldStringifyWithPathAndNameWhenNameHasNoPrecedence(): void + { + $path = new Path('field1'); + $name = new Name('field_name'); + $subject = new Subject('input', $path, $name, false); + + $testingStringifier = new TestingStringifier(); + $stringifier = new SubjectStringifier($testingStringifier); + + $pathString = $testingStringifier->stringify($path, 0); + $nameString = $testingStringifier->stringify($name, 0); + $expected = sprintf('%s (<- %s)', $pathString, $nameString); + $actual = $stringifier->stringify($subject, 0); + + self::assertSame($expected, $actual); + } + + #[Test] + public function itShouldStringifyWithNestedPathWhenNameHasNoPrecedence(): void + { + $path1 = new Path('field1'); + $path2 = new Path('field2', $path1); + $name = new Name('field_name'); + $subject = new Subject('input', $path2, $name, false); + + $testingStringifier = new TestingStringifier(); + $stringifier = new SubjectStringifier($testingStringifier); + + $pathString = $testingStringifier->stringify($path2, 0); + $nameString = $testingStringifier->stringify($name, 0); + $expected = sprintf('%s (<- %s)', $pathString, $nameString); + $actual = $stringifier->stringify($subject, 0); + + self::assertSame($expected, $actual); + } + + /** @return array */ + public static function providerForNonSubjectValues(): array + { + return [ + 'string' => ['test'], + 'integer' => [123], + 'boolean' => [true], + 'array' => [['test']], + 'object' => [new stdClass()], + 'null' => [null], + ]; + } +}