From ae1620b23d7a1b6eda242fee398c437dfee92c48 Mon Sep 17 00:00:00 2001 From: Henrique Moody Date: Sun, 28 Jan 2024 02:03:41 +0100 Subject: [PATCH] Refactor "AbstractComposite" class The class had too much complexity and some duplication on its children. I started doing this because I'm refactoring the tests to upgrade PHPUnit later, and then I made some improvements along the way. Signed-off-by: Henrique Moody --- library/Exceptions/AllOfException.php | 5 + .../Exceptions/GroupedValidationException.php | 10 - library/Rules/AbstractComposite.php | 47 ++-- library/Rules/AllOf.php | 22 +- library/Rules/AnyOf.php | 16 +- library/Rules/NoneOf.php | 15 +- library/Rules/OneOf.php | 16 +- tests/integration/translator-assert.phpt | 2 +- .../Exceptions/CompositeStubException.php | 16 ++ tests/library/Stubs/CompositeSub.php | 36 +++ tests/unit/Rules/AbstractCompositeTest.php | 222 +++++++++--------- 11 files changed, 223 insertions(+), 184 deletions(-) create mode 100644 tests/library/Exceptions/CompositeStubException.php create mode 100644 tests/library/Stubs/CompositeSub.php diff --git a/library/Exceptions/AllOfException.php b/library/Exceptions/AllOfException.php index a0140c1c5..1e8c5114e 100644 --- a/library/Exceptions/AllOfException.php +++ b/library/Exceptions/AllOfException.php @@ -24,4 +24,9 @@ class AllOfException extends GroupedValidationException self::SOME => 'These rules must not pass for {{name}}', ], ]; + + protected function chooseTemplate(): string + { + return self::SOME; + } } diff --git a/library/Exceptions/GroupedValidationException.php b/library/Exceptions/GroupedValidationException.php index fe2eb7c53..e5f0b0200 100644 --- a/library/Exceptions/GroupedValidationException.php +++ b/library/Exceptions/GroupedValidationException.php @@ -9,8 +9,6 @@ namespace Respect\Validation\Exceptions; -use function count; - class GroupedValidationException extends NestedValidationException { public const NONE = 'none'; @@ -29,12 +27,4 @@ class GroupedValidationException extends NestedValidationException self::SOME => 'These rules must not pass for {{name}}', ], ]; - - protected function chooseTemplate(): string - { - $numRules = $this->getParam('passed'); - $numFailed = count($this->getChildren()); - - return $numRules === $numFailed ? self::NONE : self::SOME; - } } diff --git a/library/Rules/AbstractComposite.php b/library/Rules/AbstractComposite.php index 8ae1c668b..d219581cd 100644 --- a/library/Rules/AbstractComposite.php +++ b/library/Rules/AbstractComposite.php @@ -13,9 +13,6 @@ use Respect\Validation\Exceptions\ValidationException; use Respect\Validation\Validatable; -use function array_filter; -use function array_map; - abstract class AbstractComposite extends AbstractRule { /** @@ -62,27 +59,37 @@ public function getRules(): array return $this->rules; } + public function assert(mixed $input): void + { + $exceptions = $this->getAllThrownExceptions($input); + if (empty($exceptions)) { + return; + } + + $exception = $this->reportError($input); + if ($exception instanceof NestedValidationException) { + $exception->addChildren($exceptions); + } + + throw $exception; + } + /** * @return ValidationException[] */ - protected function getAllThrownExceptions(mixed $input): array + private function getAllThrownExceptions(mixed $input): array { - return array_filter( - array_map( - function (Validatable $rule) use ($input): ?ValidationException { - try { - $rule->assert($input); - } catch (ValidationException $exception) { - $this->updateExceptionTemplate($exception); - - return $exception; - } - - return null; - }, - $this->getRules() - ) - ); + $exceptions = []; + foreach ($this->getRules() as $rule) { + try { + $rule->assert($input); + } catch (ValidationException $exception) { + $this->updateExceptionTemplate($exception); + $exceptions[] = $exception; + } + } + + return $exceptions; } private function shouldHaveNameOverwritten(Validatable $rule): bool diff --git a/library/Rules/AllOf.php b/library/Rules/AllOf.php index 29bfc2a20..77f8d3ebc 100644 --- a/library/Rules/AllOf.php +++ b/library/Rules/AllOf.php @@ -17,20 +17,14 @@ class AllOf extends AbstractComposite { public function assert(mixed $input): void { - $exceptions = $this->getAllThrownExceptions($input); - $numRules = count($this->getRules()); - $numExceptions = count($exceptions); - $summary = [ - 'total' => $numRules, - 'failed' => $numExceptions, - 'passed' => $numRules - $numExceptions, - ]; - if (!empty($exceptions)) { - /** @var AllOfException $allOfException */ - $allOfException = $this->reportError($input, $summary); - $allOfException->addChildren($exceptions); - - throw $allOfException; + try { + parent::assert($input); + } catch (AllOfException $exception) { + if (count($exception->getChildren()) === count($this->getRules()) && !$exception->hasCustomTemplate()) { + $exception->updateTemplate(AllOfException::NONE); + } + + throw $exception; } } diff --git a/library/Rules/AnyOf.php b/library/Rules/AnyOf.php index 69b38351e..e94d2a4e8 100644 --- a/library/Rules/AnyOf.php +++ b/library/Rules/AnyOf.php @@ -18,16 +18,12 @@ final class AnyOf extends AbstractComposite { public function assert(mixed $input): void { - $validators = $this->getRules(); - $exceptions = $this->getAllThrownExceptions($input); - $numRules = count($validators); - $numExceptions = count($exceptions); - if ($numExceptions === $numRules) { - /** @var AnyOfException $anyOfException */ - $anyOfException = $this->reportError($input); - $anyOfException->addChildren($exceptions); - - throw $anyOfException; + try { + parent::assert($input); + } catch (AnyOfException $exception) { + if (count($exception->getChildren()) === count($this->getRules())) { + throw $exception; + } } } diff --git a/library/Rules/NoneOf.php b/library/Rules/NoneOf.php index e0ab34fde..d88f96693 100644 --- a/library/Rules/NoneOf.php +++ b/library/Rules/NoneOf.php @@ -17,15 +17,12 @@ final class NoneOf extends AbstractComposite { public function assert(mixed $input): void { - $exceptions = $this->getAllThrownExceptions($input); - $numRules = count($this->getRules()); - $numExceptions = count($exceptions); - if ($numRules !== $numExceptions) { - /** @var NoneOfException $noneOfException */ - $noneOfException = $this->reportError($input); - $noneOfException->addChildren($exceptions); - - throw $noneOfException; + try { + parent::assert($input); + } catch (NoneOfException $exception) { + if (count($exception->getChildren()) !== count($this->getRules())) { + throw $exception; + } } } diff --git a/library/Rules/OneOf.php b/library/Rules/OneOf.php index 0fa0f4102..00925df14 100644 --- a/library/Rules/OneOf.php +++ b/library/Rules/OneOf.php @@ -19,16 +19,12 @@ final class OneOf extends AbstractComposite { public function assert(mixed $input): void { - $validators = $this->getRules(); - $exceptions = $this->getAllThrownExceptions($input); - $numRules = count($validators); - $numExceptions = count($exceptions); - if ($numExceptions !== $numRules - 1) { - /** @var OneOfException $oneOfException */ - $oneOfException = $this->reportError($input); - $oneOfException->addChildren($exceptions); - - throw $oneOfException; + try { + parent::assert($input); + } catch (OneOfException $exception) { + if (count($exception->getChildren()) !== count($this->getRules()) - 1) { + throw $exception; + } } } diff --git a/tests/integration/translator-assert.phpt b/tests/integration/translator-assert.phpt index 2085e1a72..6097808cf 100644 --- a/tests/integration/translator-assert.phpt +++ b/tests/integration/translator-assert.phpt @@ -20,7 +20,7 @@ Factory::setDefaultInstance( => '{{name}} deve ser do tipo string', '{{name}} must have a length between {{minValue}} and {{maxValue}}' => '{{name}} deve possuir de {{minValue}} a {{maxValue}} caracteres', - ][$message]; + ][$message] ?? $message; }) ); diff --git a/tests/library/Exceptions/CompositeStubException.php b/tests/library/Exceptions/CompositeStubException.php new file mode 100644 index 000000000..f33992d0a --- /dev/null +++ b/tests/library/Exceptions/CompositeStubException.php @@ -0,0 +1,16 @@ + + * SPDX-License-Identifier: MIT + */ + +declare(strict_types=1); + +namespace Respect\Validation\Test\Exceptions; + +use Respect\Validation\Exceptions\NestedValidationException; + +final class CompositeStubException extends NestedValidationException +{ +} diff --git a/tests/library/Stubs/CompositeSub.php b/tests/library/Stubs/CompositeSub.php new file mode 100644 index 000000000..a82d5e05c --- /dev/null +++ b/tests/library/Stubs/CompositeSub.php @@ -0,0 +1,36 @@ + + * SPDX-License-Identifier: MIT + */ + +declare(strict_types=1); + +namespace Respect\Validation\Test\Stubs; + +use Respect\Validation\Message\Formatter; +use Respect\Validation\Message\Stringifier\KeepOriginalStringName; +use Respect\Validation\Rules\AbstractComposite; +use Respect\Validation\Test\Exceptions\CompositeStubException; + +final class CompositeSub extends AbstractComposite +{ + public function validate(mixed $input): bool + { + return true; + } + + /** + * @param array $extraParameters + */ + public function reportError(mixed $input, array $extraParameters = []): CompositeStubException + { + return new CompositeStubException( + input: $input, + id: 'CompositeStub', + params: $extraParameters, + formatter: new Formatter(static fn ($value) => $value, new KeepOriginalStringName()) + ); + } +} diff --git a/tests/unit/Rules/AbstractCompositeTest.php b/tests/unit/Rules/AbstractCompositeTest.php index 7382f9cf1..0460e3540 100644 --- a/tests/unit/Rules/AbstractCompositeTest.php +++ b/tests/unit/Rules/AbstractCompositeTest.php @@ -9,8 +9,12 @@ namespace Respect\Validation\Rules; +use Respect\Validation\Test\Exceptions\CompositeStubException; +use Respect\Validation\Test\Rules\Stub; +use Respect\Validation\Test\Stubs\CompositeSub; use Respect\Validation\Test\TestCase; -use Respect\Validation\Validatable; + +use function current; /** * @group rule @@ -21,175 +25,173 @@ final class AbstractCompositeTest extends TestCase /** * @test */ - public function shouldDefineNameForInternalWhenAppendRuleToCompositeRule(): void + public function itShouldUpdateTheNameOfTheChildWhenUpdatingItsName(): void { $ruleName = 'something'; - $rule = $this->createMock(Validatable::class); - $rule - ->expects(self::once()) - ->method('getName') - ->will(self::returnValue(null)); - $rule - ->expects(self::once()) - ->method('setName') - ->with($ruleName); + $child = new Stub(); - $sut = $this - ->getMockBuilder(AbstractComposite::class) - ->getMockForAbstractClass(); + $parent = new CompositeSub($child); - $sut->setName($ruleName); - $sut->addRule($rule); + self::assertNull($child->getName()); + + $parent->setName($ruleName); + + self::assertSame($ruleName, $child->getName()); } /** * @test */ - public function shouldUpdateInternalRuleNameWhenNameIsUpdated(): void + public function itShouldUpdateTheNameOfTheChildWhenAddingIt(): void { - $ruleName1 = 'something'; - $ruleName2 = 'something else'; + $ruleName = 'something'; - $rule = $this->createMock(Validatable::class); - $rule - ->expects(self::exactly(2)) - ->method('getName') - ->willReturnOnConsecutiveCalls( - null, - $ruleName1 - ); + $rule = new Stub(); + + $sut = new CompositeSub(); + $sut->setName($ruleName); - $sut = $this - ->getMockBuilder(AbstractComposite::class) - ->getMockForAbstractClass(); + self::assertNull($rule->getName()); - $sut->setName($ruleName1); $sut->addRule($rule); - $sut->setName($ruleName2); + + self::assertSame($ruleName, $rule->getName()); } /** * @test */ - public function shouldNotUpdateInternalRuleWhenItAlreadyHasName(): void + public function itShouldNotUpdateTheNameOfTheChildWhenUpdatingItsNameIfTheChildAlreadyHasSomeName(): void { - $rule = $this->createMock(Validatable::class); - $rule - ->expects(self::any()) - ->method('getName') - ->will(self::returnValue('something')); - $rule - ->expects(self::never()) - ->method('setName'); - - $sut = $this - ->getMockBuilder(AbstractComposite::class) - ->getMockForAbstractClass(); - $sut->addRule($rule); - $sut->setName('Whatever'); + $ruleName1 = 'something'; + $ruleName2 = 'something else'; + + $rule = new Stub(); + $rule->setName($ruleName1); + + $sut = new CompositeSub($rule); + $sut->setName($ruleName2); + + self::assertSame($ruleName1, $rule->getName()); } /** * @test */ - public function shouldUpdateInternalRuleWhenItsNameIsNull(): void + public function itNotShouldUpdateTheNameOfTheChildWhenAddingItIfTheChildAlreadyHasSomeName(): void { - $ruleName = 'something'; - - $rule = $this->createMock(Validatable::class); - $rule - ->expects(self::any()) - ->method('getName') - ->will(self::returnValue(null)); - $rule - ->expects(self::once()) - ->method('setName') - ->with($ruleName); + $ruleName1 = 'something'; + $ruleName2 = 'something else'; - $sut = $this - ->getMockBuilder(AbstractComposite::class) - ->getMockForAbstractClass(); + $rule = new Stub(); + $rule->setName($ruleName1); + $sut = new CompositeSub(); + $sut->setName($ruleName2); $sut->addRule($rule); - $sut->setName($ruleName); + + self::assertSame($ruleName1, $rule->getName()); } /** * @test */ - public function shouldDefineNameForInternalRulesWhenItDoesNotHaveName(): void + public function itShouldReturnItsChildren(): void { - $ruleName = 'something'; - - $rule = $this->createMock(Validatable::class); - $rule - ->expects(self::any()) - ->method('getName') - ->will(self::returnValue(null)); - $rule - ->expects(self::once()) - ->method('setName') - ->with($ruleName); + $child1 = new Stub(); + $child2 = new Stub(); + $child3 = new Stub(); - $sut = $this - ->getMockBuilder(AbstractComposite::class) - ->getMockForAbstractClass(); + $sut = new CompositeSub($child1, $child2, $child3); + $children = $sut->getRules(); - $sut->addRule($rule); - $sut->setName($ruleName); + self::assertCount(3, $children); + self::assertSame($child1, $children[0]); + self::assertSame($child2, $children[1]); + self::assertSame($child3, $children[2]); } /** * @test */ - public function shouldNotDefineNameForInternalRulesWhenItAlreadyHasName(): void + public function itShouldAssertWithAllChildrenAndNotThrowAnExceptionWhenThereAreNoIssues(): void { - $ruleName = 'something'; + $input = 'something'; - $rule = $this->createMock(Validatable::class); - $rule - ->expects(self::any()) - ->method('getName') - ->will(self::returnValue($ruleName)); - $rule - ->expects(self::never()) - ->method('setName'); + $child1 = new Stub(true); + $child2 = new Stub(true); + $child3 = new Stub(true); - $sut = $this - ->getMockBuilder(AbstractComposite::class) - ->getMockForAbstractClass(); + $this->expectNotToPerformAssertions(); - $sut->addRule($rule); - $sut->setName($ruleName); + $sut = new CompositeSub($child1, $child2, $child3); + $sut->assert($input); } /** * @test */ - public function shouldReturnTheAmountOfAddedRules(): void + public function itShouldAssertWithAllChildrenAndThrowAnExceptionWhenThereAreIssues(): void { - $sut = $this->getMockForAbstractClass(AbstractComposite::class); - $sut->addRule($this->createMock(Validatable::class)); - $sut->addRule($this->createMock(Validatable::class)); - $sut->addRule($this->createMock(Validatable::class)); + $sut = new CompositeSub(new Stub(false), new Stub(false), new Stub(false)); - self::assertCount(3, $sut->getRules()); + try { + $sut->assert('something'); + } catch (CompositeStubException $exception) { + self::assertCount(3, $exception->getChildren()); + } } /** * @test */ - public function shouldAddRulesByPassingThroughConstructor(): void + public function itShouldUpdateTheTemplateOfEveryChildrenWhenAsserting(): void { - $rule = $this->createMock(Validatable::class); - $anotherSimpleRuleMock = $this->createMock(Validatable::class); - - $sut = $this->getMockForAbstractClass(AbstractComposite::class, [ - $rule, - $anotherSimpleRuleMock, - ]); + $template = 'This is my template'; + + $sut = new CompositeSub( + new Stub(false), + new Stub(false), + new Stub(false) + ); + $sut->setTemplate($template); + + try { + $sut->assert('something'); + } catch (CompositeStubException $exception) { + foreach ($exception->getChildren() as $child) { + self::assertEquals($template, $child->getMessage()); + } + } + } - self::assertCount(2, $sut->getRules()); + /** + * @test + */ + public function itShouldUpdateTheTemplateOfEveryTheChildrenOfSomeChildWhenAsserting(): void + { + $template = 'This is my template'; + + $sut = new CompositeSub( + new Stub(false), + new Stub(false), + new CompositeSub(new Stub(false)) + ); + $sut->setTemplate($template); + + try { + $sut->assert('something'); + } catch (CompositeStubException $exception) { + foreach ($exception->getChildren() as $child) { + self::assertEquals($template, $child->getMessage()); + if (!$child instanceof CompositeStubException) { + continue; + } + + self::assertNotFalse(current($child->getChildren())); + self::assertEquals($template, current($child->getChildren())->getMessage()); + } + } } }