From 580a02b78eaa4382c6ca4ddadbcd074700d07ea7 Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Wed, 26 Jul 2023 21:58:12 +0200 Subject: [PATCH 1/8] Create a And expression This will allow for complex nested expressions, like ANDs inside ORs. --- src/Expression/Boolean/Andx.php | 56 +++++++++++ tests/Unit/Expressions/Boolean/AndxTest.php | 105 ++++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 src/Expression/Boolean/Andx.php create mode 100644 tests/Unit/Expressions/Boolean/AndxTest.php diff --git a/src/Expression/Boolean/Andx.php b/src/Expression/Boolean/Andx.php new file mode 100644 index 00000000..a4a8c329 --- /dev/null +++ b/src/Expression/Boolean/Andx.php @@ -0,0 +1,56 @@ +expressions = $expressions; + } + + public function describe(ClassDescription $theClass, string $because): Description + { + $expressionsDescriptions = []; + foreach ($this->expressions as $expression) { + $expressionsDescriptions[] = $expression->describe($theClass, '')->toString(); + } + + return new Description( + 'all expressions must be true ('.implode(', ', $expressionsDescriptions).')', + $because + ); + } + + public function evaluate(ClassDescription $theClass, Violations $violations, string $because): void + { + foreach ($this->expressions as $expression) { + $newViolations = new Violations(); + $expression->evaluate($theClass, $newViolations, $because); + if (0 !== $newViolations->count()) { + $violations->add(Violation::create( + $theClass->getFQCN(), + ViolationMessage::withDescription( + $this->describe($theClass, $because), + "The class '".$theClass->getFQCN()."' violated the expression " + .$expression->describe($theClass, '')->toString() + ) + )); + + return; + } + } + } +} diff --git a/tests/Unit/Expressions/Boolean/AndxTest.php b/tests/Unit/Expressions/Boolean/AndxTest.php new file mode 100644 index 00000000..1af18c05 --- /dev/null +++ b/tests/Unit/Expressions/Boolean/AndxTest.php @@ -0,0 +1,105 @@ +evaluate($classDescription, $violations, $because); + + self::assertEquals(0, $violations->count()); + } + + public function test_it_should_pass_the_rule_when_and_is_empty(): void + { + $interface = 'interface'; + $class = 'SomeClass'; + $classDescription = new ClassDescription( + FullyQualifiedClassName::fromString('HappyIsland'), + [], + [FullyQualifiedClassName::fromString($interface)], + FullyQualifiedClassName::fromString($class), + false, + false, + false, + false, + false + ); + $andConstraint = new Andx(); + + $because = 'reasons'; + $violations = new Violations(); + $andConstraint->evaluate($classDescription, $violations, $because); + + self::assertEquals(0, $violations->count()); + } + + public function test_it_should_not_pass_the_rule(): void + { + $interface = 'SomeInterface'; + $class = 'SomeClass'; + + $classDescription = new ClassDescription( + FullyQualifiedClassName::fromString('HappyIsland'), + [], + [FullyQualifiedClassName::fromString($interface)], + null, + false, + false, + false, + false, + false + ); + + $implementConstraint = new Implement($interface); + $extendsConstraint = new Extend($class); + $andConstraint = new Andx($implementConstraint, $extendsConstraint); + + $because = 'reasons'; + $violationError = $andConstraint->describe($classDescription, $because)->toString(); + + $violations = new Violations(); + $andConstraint->evaluate($classDescription, $violations, $because); + self::assertNotEquals(0, $violations->count()); + + $this->assertEquals( + 'all expressions must be true (should implement SomeInterface, should extend SomeClass) because reasons', + $violationError + ); + $this->assertEquals( + "The class 'HappyIsland' violated the expression should extend SomeClass, but " + .'all expressions must be true (should implement SomeInterface, should extend SomeClass) because reasons', + $violations->get(0)->getError() + ); + } +} From 5d965f49c32093dd1116f584125bf8f0ed289bed Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Wed, 26 Jul 2023 21:31:48 +0200 Subject: [PATCH 2/8] Create a Not expression This will reduce the amount of expressions needed and allow for complex expressions (within an OR or an AND) to be negated. --- src/Expression/Boolean/Not.php | 42 +++++++++++++++ tests/Unit/Expressions/Boolean/NotTest.php | 59 ++++++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 src/Expression/Boolean/Not.php create mode 100644 tests/Unit/Expressions/Boolean/NotTest.php diff --git a/src/Expression/Boolean/Not.php b/src/Expression/Boolean/Not.php new file mode 100644 index 00000000..3db76654 --- /dev/null +++ b/src/Expression/Boolean/Not.php @@ -0,0 +1,42 @@ +expression = $expression; + } + + public function describe(ClassDescription $theClass, string $because): Description + { + return new Description('must NOT ('.$this->expression->describe($theClass, '')->toString().')', $because); + } + + public function evaluate(ClassDescription $theClass, Violations $violations, string $because): void + { + $newViolations = new Violations(); + $this->expression->evaluate($theClass, $newViolations, $because); + if (0 !== $newViolations->count()) { + return; + } + + $violations->add(Violation::create( + $theClass->getFQCN(), + ViolationMessage::selfExplanatory($this->describe($theClass, $because)) + )); + } +} diff --git a/tests/Unit/Expressions/Boolean/NotTest.php b/tests/Unit/Expressions/Boolean/NotTest.php new file mode 100644 index 00000000..ac844d8a --- /dev/null +++ b/tests/Unit/Expressions/Boolean/NotTest.php @@ -0,0 +1,59 @@ +describe($classDescription, $because)->toString(); + + $violations = new Violations(); + $isNotInterface->evaluate($classDescription, $violations, $because); + self::assertNotEquals(0, $violations->count()); + + $this->assertEquals('must NOT (HappyIsland should be an interface) because we want to add this rule for our software', $violationError); + } + + public function test_it_should_return_true_if_is_not_interface(): void + { + $isNotInterface = new Not(new IsInterface()); + $classDescription = new ClassDescription( + FullyQualifiedClassName::fromString('HappyIsland'), + [], + [], + null, + false, + false, + false, + false, + false + ); + $because = 'we want to add this rule for our software'; + $violations = new Violations(); + $isNotInterface->evaluate($classDescription, $violations, $because); + self::assertEquals(0, $violations->count()); + } +} From 92f7cd6944ee247f0af0c70b6d56f863b39d9f95 Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Mon, 11 Sep 2023 10:13:21 +0200 Subject: [PATCH 3/8] Add doc-blocks To improve understanding of mayDependOnComponents and shouldOnlyDependOnComponents --- src/RuleBuilders/Architecture/MayDependOnComponents.php | 8 +++++++- .../Architecture/ShouldOnlyDependOnComponents.php | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/RuleBuilders/Architecture/MayDependOnComponents.php b/src/RuleBuilders/Architecture/MayDependOnComponents.php index d9e9a7e5..70f004bf 100644 --- a/src/RuleBuilders/Architecture/MayDependOnComponents.php +++ b/src/RuleBuilders/Architecture/MayDependOnComponents.php @@ -5,6 +5,12 @@ interface MayDependOnComponents { - /** @return Where&Rules */ + /** + * May depend on the specified components, plus itself. + * + * @param string[] $componentNames + * + * @return Where&Rules + */ public function mayDependOnComponents(string ...$componentNames); } diff --git a/src/RuleBuilders/Architecture/ShouldOnlyDependOnComponents.php b/src/RuleBuilders/Architecture/ShouldOnlyDependOnComponents.php index 021a32d0..f1f2a625 100644 --- a/src/RuleBuilders/Architecture/ShouldOnlyDependOnComponents.php +++ b/src/RuleBuilders/Architecture/ShouldOnlyDependOnComponents.php @@ -5,6 +5,12 @@ interface ShouldOnlyDependOnComponents { - /** @return Where&Rules */ + /** + * May depend ONLY on the specified components, thus it can only depend on itself if itself is specified. + * + * @param string[] $componentNames + * + * @return Where&Rules + */ public function shouldOnlyDependOnComponents(string ...$componentNames); } From 7cab8ff1950d3ade0f628ebb8accdfdb08276a72 Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Sat, 16 Sep 2023 17:39:33 +0100 Subject: [PATCH 4/8] Prevent duplicate namespaces in ResideInOneOfTheseNamespaces --- src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php b/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php index 5048486d..92578b71 100644 --- a/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php +++ b/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php @@ -18,7 +18,7 @@ class ResideInOneOfTheseNamespaces implements Expression public function __construct(string ...$namespaces) { - $this->namespaces = $namespaces; + $this->namespaces = array_unique($namespaces); } public function describe(ClassDescription $theClass, string $because): Description From e5e7140835d9157c849ac0633b57138ccb66a94f Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Sat, 16 Sep 2023 13:51:53 +0200 Subject: [PATCH 5/8] Make ResideInOneOfTheseNamespaces mergeable This simplifies expressions and makes them more predictable. --- .php-cs-fixer.dist.php | 81 +++++++++++-------- .../ResideInOneOfTheseNamespaces.php | 12 ++- src/Expression/MergeableExpression.php | 13 +++ 3 files changed, 72 insertions(+), 34 deletions(-) create mode 100644 src/Expression/MergeableExpression.php diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php index 64388f3f..bb8c5e06 100644 --- a/.php-cs-fixer.dist.php +++ b/.php-cs-fixer.dist.php @@ -5,40 +5,55 @@ ->exclude('vendor/') ->notPath('tests/E2E/_fixtures/parse_error/Services/CartService.php'); +$rules = [ + '@Symfony' => true, + '@Symfony:risky' => true, + '@PHP71Migration:risky' => true, + '@PSR2' => true, + '@DoctrineAnnotation' => true, + 'array_syntax' => ['syntax' => 'short'], + 'fully_qualified_strict_types' => true, // Transforms imported FQCN parameters and return types in function arguments to short version. + 'dir_constant' => true, // Replaces dirname(__FILE__) expression with equivalent __DIR__ constant. + 'heredoc_to_nowdoc' => true, + 'linebreak_after_opening_tag' => true, // Ensure there is no code on the same line as the PHP open tag. + 'blank_line_after_opening_tag' => false, + 'modernize_types_casting' => true, // Replaces intval, floatval, doubleval, strval and boolval function calls with according type casting operator. + 'multiline_whitespace_before_semicolons' => true, // Forbid multi-line whitespace before the closing semicolon or move the semicolon to the new line for chained calls. + 'no_unreachable_default_argument_value' => true, // In function arguments there must not be arguments with default values before non-default ones. + 'no_superfluous_phpdoc_tags' => ['allow_mixed' => true],// To avoid problems of compatibility with the old php-cs-fixer version used on PHP 7.3 + 'no_useless_else' => true, + 'no_useless_return' => true, + 'ordered_class_elements' => true, // Orders the elements of classes/interfaces/traits. + 'ordered_imports' => true, + 'phpdoc_add_missing_param_annotation' => ['only_untyped' => false], // PHPDoc should contain @param for all params (for untyped parameters only). + 'phpdoc_order' => true, // Annotations in PHPDoc should be ordered so that @param annotations come first, then @throws annotations, then @return annotations. + 'declare_strict_types' => true, + 'psr_autoloading' => true, // Class names should match the file name. + 'no_php4_constructor' => true, // Convert PHP4-style constructors to __construct. + 'semicolon_after_instruction' => true, + 'align_multiline_comment' => true, + 'general_phpdoc_annotation_remove' => ['annotations' => ['author', 'package']], + 'list_syntax' => ['syntax' => 'short'], + 'phpdoc_to_comment' => false, + 'php_unit_method_casing' => ['case' => 'snake_case'], + 'function_to_constant' => false, + 'native_constant_invocation' =>[ + 'fix_built_in' => false, + 'include' => [ + 'DIRECTORY_SEPARATOR', + 'PHP_INT_SIZE', + 'PHP_SAPI', + 'PHP_VERSION_ID' + ], + 'scope' => 'namespaced', + 'strict' => false + ] +]; +if (phpversion()[0] == '8') { + $rules['get_class_to_class_keyword'] = false; // if enabled, either if fails on PHP8, or it fails on PHP7 +} return (new PhpCsFixer\Config()) ->setFinder($finder) ->setRiskyAllowed(true) - ->setRules([ - '@Symfony' => true, - '@Symfony:risky' => true, - '@PHP71Migration:risky' => true, - '@PSR2' => true, - '@DoctrineAnnotation' => true, - 'array_syntax' => ['syntax' => 'short'], - 'fully_qualified_strict_types' => true, // Transforms imported FQCN parameters and return types in function arguments to short version. - 'dir_constant' => true, // Replaces dirname(__FILE__) expression with equivalent __DIR__ constant. - 'heredoc_to_nowdoc' => true, - 'linebreak_after_opening_tag' => true, // Ensure there is no code on the same line as the PHP open tag. - 'blank_line_after_opening_tag' => false, - 'modernize_types_casting' => true, // Replaces intval, floatval, doubleval, strval and boolval function calls with according type casting operator. - 'multiline_whitespace_before_semicolons' => true, // Forbid multi-line whitespace before the closing semicolon or move the semicolon to the new line for chained calls. - 'no_unreachable_default_argument_value' => true, // In function arguments there must not be arguments with default values before non-default ones. - 'no_superfluous_phpdoc_tags' => ['allow_mixed' => true],// To avoid problems of compatibility with the old php-cs-fixer version used on PHP 7.3 - 'no_useless_else' => true, - 'no_useless_return' => true, - 'ordered_class_elements' => true, // Orders the elements of classes/interfaces/traits. - 'ordered_imports' => true, - 'phpdoc_add_missing_param_annotation' => ['only_untyped' => false], // PHPDoc should contain @param for all params (for untyped parameters only). - 'phpdoc_order' => true, // Annotations in PHPDoc should be ordered so that @param annotations come first, then @throws annotations, then @return annotations. - 'declare_strict_types' => true, - 'psr_autoloading' => true, // Class names should match the file name. - 'no_php4_constructor' => true, // Convert PHP4-style constructors to __construct. - 'semicolon_after_instruction' => true, - 'align_multiline_comment' => true, - 'general_phpdoc_annotation_remove' => ['annotations' => ['author', 'package']], - 'list_syntax' => ['syntax' => 'short'], - 'phpdoc_to_comment' => false, - 'php_unit_method_casing' => ['case' => 'snake_case'], - 'function_to_constant' => false - ]); + ->setRules($rules); diff --git a/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php b/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php index 92578b71..431aa819 100644 --- a/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php +++ b/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php @@ -7,11 +7,12 @@ use Arkitect\Analyzer\ClassDescription; use Arkitect\Expression\Description; use Arkitect\Expression\Expression; +use Arkitect\Expression\MergeableExpression; use Arkitect\Rules\Violation; use Arkitect\Rules\ViolationMessage; use Arkitect\Rules\Violations; -class ResideInOneOfTheseNamespaces implements Expression +class ResideInOneOfTheseNamespaces implements Expression, MergeableExpression { /** @var string[] */ private $namespaces; @@ -45,4 +46,13 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str $violations->add($violation); } } + + public function mergeWith(Expression $expression): Expression + { + if (!$expression instanceof self) { + throw new \InvalidArgumentException('Can not merge expressions. The given expression should be of type '.\get_class($this).' but is of type '.\get_class($expression)); + } + + return new self(...array_merge($this->namespaces, $expression->namespaces)); + } } diff --git a/src/Expression/MergeableExpression.php b/src/Expression/MergeableExpression.php new file mode 100644 index 00000000..e1204778 --- /dev/null +++ b/src/Expression/MergeableExpression.php @@ -0,0 +1,13 @@ + Date: Sat, 16 Sep 2023 17:40:37 +0100 Subject: [PATCH 6/8] Create DependsOnlyOnTheseExpressions --- .../DependsOnlyOnTheseExpressions.php | 155 ++++++++++++++++++ .../DependsOnlyOnTheseExpressionsTest.php | 62 +++++++ 2 files changed, 217 insertions(+) create mode 100644 src/Expression/ForClasses/DependsOnlyOnTheseExpressions.php create mode 100644 tests/Unit/Expressions/ForClasses/DependsOnlyOnTheseExpressionsTest.php diff --git a/src/Expression/ForClasses/DependsOnlyOnTheseExpressions.php b/src/Expression/ForClasses/DependsOnlyOnTheseExpressions.php new file mode 100644 index 00000000..31ce0778 --- /dev/null +++ b/src/Expression/ForClasses/DependsOnlyOnTheseExpressions.php @@ -0,0 +1,155 @@ +fileParser = FileParserFactory::createFileParser(); + foreach ($expressions as $newExpression) { + $this->addExpression($newExpression); + } + } + + public function describe(ClassDescription $theClass, string $because): Description + { + $expressionsDescriptions = ''; + foreach ($this->expressions as $expression) { + $expressionsDescriptions .= $expression->describe($theClass, '')->toString()."\n"; + } + + return new Description( + "should depend only on classes in one of the given expressions: \n" + .$expressionsDescriptions, + $because + ); + } + + public function evaluate(ClassDescription $theClass, Violations $violations, string $because): void + { + $dependencies = $this->removeDuplicateDependencies($theClass->getDependencies()); + + foreach ($dependencies as $dependency) { + if ( + '' === $dependency->getFQCN()->namespace() + || $theClass->namespaceMatches($dependency->getFQCN()->namespace()) + ) { + continue; + } + + $dependencyClassDescription = $this->getDependencyClassDescription($dependency); + if (null === $dependencyClassDescription) { + return; + } + + if (!$this->matchesAnyOfTheExpressions($dependencyClassDescription)) { + $violations->add( + Violation::create( + $theClass->getFQCN(), + ViolationMessage::withDescription( + $this->describe($theClass, $because), + "The dependency '".$dependencyClassDescription->getFQCN()."' violated the expression: \n" + .$this->describeDependencyRequirement($dependencyClassDescription)."\n" + ) + ) + ); + } + } + } + + private function describeDependencyRequirement(ClassDescription $theDependency): string + { + $expressionsDescriptions = []; + foreach ($this->expressions as $expression) { + $expressionsDescriptions[] = $expression->describe($theDependency, '')->toString(); + } + + return implode("\nOR\n", array_unique($expressionsDescriptions)); + } + + private function matchesAnyOfTheExpressions(ClassDescription $dependencyClassDescription): bool + { + foreach ($this->expressions as $expression) { + $newViolations = new Violations(); + $expression->evaluate($dependencyClassDescription, $newViolations, ''); + if (0 === $newViolations->count()) { + return true; + } + } + + return false; + } + + /** + * @param ClassDependency $dependency + */ + private function getDependencyClassDescription($dependency): ?ClassDescription + { + /** @var class-string $dependencyFqcn */ + $dependencyFqcn = $dependency->getFQCN()->toString(); + $reflector = new \ReflectionClass($dependencyFqcn); + $filename = $reflector->getFileName(); + if (false === $filename) { + return null; + } + $this->fileParser->parse(file_get_contents($filename), $filename); + $classDescriptionList = $this->fileParser->getClassDescriptions(); + + return array_pop($classDescriptionList); + } + + /** + * @param ClassDependency[] $dependenciesList + * + * @return ClassDependency[] + */ + private function removeDuplicateDependencies(array $dependenciesList): array + { + $filteredList = []; + foreach ($dependenciesList as $classDependency) { + $dependencyFqcn = $classDependency->getFQCN()->toString(); + if (\array_key_exists($dependencyFqcn, $filteredList)) { + continue; + } + $filteredList[$dependencyFqcn] = $classDependency; + } + + return $filteredList; + } + + private function addExpression(Expression $newExpression): void + { + foreach ($this->expressions as $index => $existingExpression) { + if ( + $newExpression instanceof $existingExpression + && $newExpression instanceof MergeableExpression + && $existingExpression instanceof MergeableExpression + ) { + $this->expressions[$index] = $existingExpression->mergeWith($newExpression); + + return; + } + } + $this->expressions[] = $newExpression; + } +} diff --git a/tests/Unit/Expressions/ForClasses/DependsOnlyOnTheseExpressionsTest.php b/tests/Unit/Expressions/ForClasses/DependsOnlyOnTheseExpressionsTest.php new file mode 100644 index 00000000..1fc9c729 --- /dev/null +++ b/tests/Unit/Expressions/ForClasses/DependsOnlyOnTheseExpressionsTest.php @@ -0,0 +1,62 @@ +build(); + $because = 'we want to add this rule for our software'; + $violations = new Violations(); + $dependsOnlyOnTheseExpressions->evaluate($classDescription, $violations, $because); + + self::assertEquals(0, $violations->count()); + } + + public function test_it_should_have_no_violations_if_it_has_no_dependencies_outside_expression(): void + { + $dependsOnlyOnTheseExpressions = new DependsOnlyOnTheseExpressions(new ResideInOneOfTheseNamespaces('Arkitect\Tests\Fixtures\Fruit')); + + $classDescription = $this->getClassDescription(file_get_contents(\FIXTURES_PATH.'/Fruit/Banana.php')); + + $because = 'we want to add this rule for our software'; + $violations = new Violations(); + $dependsOnlyOnTheseExpressions->evaluate($classDescription, $violations, $because); + + self::assertEquals(0, $violations->count()); + } + + public function test_it_should_have_violations_if_it_has_dependencies_outside_expression(): void + { + $dependsOnlyOnTheseExpressions = new DependsOnlyOnTheseExpressions(new ResideInOneOfTheseNamespaces('Arkitect\Tests\Fixtures\Fruit')); + + $classDescription = $this->getClassDescription(file_get_contents(\FIXTURES_PATH.'/Fruit/AnimalFruit.php')); + + $because = 'we want to add this rule for our software'; + $violations = new Violations(); + $dependsOnlyOnTheseExpressions->evaluate($classDescription, $violations, $because); + + self::assertEquals(1, $violations->count()); + } + + private function getClassDescription(string $classCode, string $fileName = 'MyClass.php'): ClassDescription + { + $fileParser = FileParserFactory::createFileParser(); + $fileParser->parse($classCode, $fileName); + $classDescriptionList = $fileParser->getClassDescriptions(); + + return array_pop($classDescriptionList); + } +} From 972b3058c59e75b272e48d53165b7db35ebdc85d Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Thu, 27 Jul 2023 14:50:13 +0200 Subject: [PATCH 7/8] Allow architectures to be created from expressions This will allow for composable and more complex rules. --- .../Architecture/Architecture.php | 49 ++++++++++++-- src/RuleBuilders/Architecture/DefinedBy.php | 5 ++ tests/Unit/Architecture/ArchitectureTest.php | 67 +++++++++++++++++++ 3 files changed, 115 insertions(+), 6 deletions(-) diff --git a/src/RuleBuilders/Architecture/Architecture.php b/src/RuleBuilders/Architecture/Architecture.php index a1b38832..ab23c935 100644 --- a/src/RuleBuilders/Architecture/Architecture.php +++ b/src/RuleBuilders/Architecture/Architecture.php @@ -3,6 +3,9 @@ namespace Arkitect\RuleBuilders\Architecture; +use Arkitect\Expression\Boolean\Andx; +use Arkitect\Expression\Boolean\Not; +use Arkitect\Expression\Expression; use Arkitect\Expression\ForClasses\DependsOnlyOnTheseNamespaces; use Arkitect\Expression\ForClasses\NotDependsOnTheseNamespaces; use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces; @@ -46,6 +49,13 @@ public function definedBy(string $selector) return $this; } + public function definedByExpression(Expression $selector) + { + $this->componentSelectors[$this->componentName] = $selector; + + return $this; + } + public function where(string $componentName) { $this->componentName = $componentName; @@ -90,13 +100,9 @@ public function rules(): iterable $forbiddenComponents = array_diff($layerNames, [$name], $this->allowedDependencies[$name]); if (!empty($forbiddenComponents)) { - $forbiddenSelectors = array_map(function (string $componentName): string { - return $this->componentSelectors[$componentName]; - }, $forbiddenComponents); - yield Rule::allClasses() - ->that(new ResideInOneOfTheseNamespaces($selector)) - ->should(new NotDependsOnTheseNamespaces(...$forbiddenSelectors)) + ->that(\is_string($selector) ? new ResideInOneOfTheseNamespaces($selector) : $selector) + ->should($this->createForbiddenExpression($forbiddenComponents)) ->because('of component architecture'); } } @@ -115,4 +121,35 @@ public function rules(): iterable ->because('of component architecture'); } } + + public function createForbiddenExpression(array $forbiddenComponents): Expression + { + $forbiddenNamespaceSelectors = array_filter( + array_map(function (string $componentName): ?string { + $selector = $this->componentSelectors[$componentName]; + + return \is_string($selector) ? $selector : null; + }, $forbiddenComponents) + ); + + $forbiddenExpressionSelectors = array_filter( + array_map(function (string $componentName): ?Expression { + $selector = $this->componentSelectors[$componentName]; + + return \is_string($selector) ? null : $selector; + }, $forbiddenComponents) + ); + + $forbiddenExpressionList = []; + if ([] !== $forbiddenNamespaceSelectors) { + $forbiddenExpressionList[] = new NotDependsOnTheseNamespaces(...$forbiddenNamespaceSelectors); + } + if ([] !== $forbiddenExpressionSelectors) { + $forbiddenExpressionList[] = new Not(new Andx(...$forbiddenExpressionSelectors)); + } + + return 1 === \count($forbiddenExpressionList) + ? array_pop($forbiddenExpressionList) + : new Andx(...$forbiddenExpressionList); + } } diff --git a/src/RuleBuilders/Architecture/DefinedBy.php b/src/RuleBuilders/Architecture/DefinedBy.php index a695f135..4bebf488 100644 --- a/src/RuleBuilders/Architecture/DefinedBy.php +++ b/src/RuleBuilders/Architecture/DefinedBy.php @@ -3,8 +3,13 @@ namespace Arkitect\RuleBuilders\Architecture; +use Arkitect\Expression\Expression; + interface DefinedBy { /** @return Component&Where */ public function definedBy(string $selector); + + /** @return Component&Where */ + public function definedByExpression(Expression $selector); } diff --git a/tests/Unit/Architecture/ArchitectureTest.php b/tests/Unit/Architecture/ArchitectureTest.php index b4f484f3..4c810be4 100644 --- a/tests/Unit/Architecture/ArchitectureTest.php +++ b/tests/Unit/Architecture/ArchitectureTest.php @@ -3,6 +3,8 @@ namespace Arkitect\Tests\Unit\Architecture; +use Arkitect\Expression\Boolean\Andx; +use Arkitect\Expression\Boolean\Not; use Arkitect\Expression\ForClasses\DependsOnlyOnTheseNamespaces; use Arkitect\Expression\ForClasses\NotDependsOnTheseNamespaces; use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces; @@ -39,6 +41,71 @@ public function test_layered_architecture(): void self::assertEquals($expectedRules, iterator_to_array($rules)); } + public function test_layered_architecture_with_expression(): void + { + $rules = Architecture::withComponents() + ->component('Domain')->definedByExpression(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) + ->component('Application')->definedByExpression(new ResideInOneOfTheseNamespaces('App\*\Application\*')) + ->component('Infrastructure') + ->definedByExpression(new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*')) + + ->where('Domain')->shouldNotDependOnAnyComponent() + ->where('Application')->mayDependOnComponents('Domain') + ->where('Infrastructure')->mayDependOnAnyComponent() + + ->rules(); + + $expectedRules = [ + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) + ->should(new Not(new Andx( + new ResideInOneOfTheseNamespaces('App\*\Application\*'), + new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*') + ))) + ->because('of component architecture'), + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Application\*')) + ->should(new Not(new Andx( + new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*') + ))) + ->because('of component architecture'), + ]; + + self::assertEquals($expectedRules, iterator_to_array($rules)); + } + + public function test_layered_architecture_with_mix_of_namespace_and_expression(): void + { + $rules = Architecture::withComponents() + ->component('Domain')->definedByExpression(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) + ->component('Application')->definedByExpression(new ResideInOneOfTheseNamespaces('App\*\Application\*')) + ->component('Infrastructure')->definedBy('App\*\Infrastructure\*') + + ->where('Domain')->shouldNotDependOnAnyComponent() + ->where('Application')->mayDependOnComponents('Domain') + ->where('Infrastructure')->mayDependOnAnyComponent() + + ->rules(); + + $expectedRules = [ + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) + ->should(new Andx( + new NotDependsOnTheseNamespaces('App\*\Infrastructure\*'), + new Not(new Andx( + new ResideInOneOfTheseNamespaces('App\*\Application\*') + )) + )) + ->because('of component architecture'), + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Application\*')) + ->should(new NotDependsOnTheseNamespaces('App\*\Infrastructure\*')) + ->because('of component architecture'), + ]; + + self::assertEquals($expectedRules, iterator_to_array($rules)); + } + public function test_layered_architecture_with_depends_only_on_components(): void { $rules = Architecture::withComponents() From 2f39bc548379a241dcf33bf5af2f8bac79e31ef5 Mon Sep 17 00:00:00 2001 From: Herberto Graca Date: Sat, 16 Sep 2023 17:44:41 +0200 Subject: [PATCH 8/8] fixup! --- .../Architecture/Architecture.php | 84 +++++++++---------- tests/Fixtures/Fruit/AnimalFruit.php | 20 +++++ tests/Unit/Architecture/ArchitectureTest.php | 66 ++++++++++----- tests/Unit/Rules/RuleCheckerTest.php | 78 +++++++++++++++++ 4 files changed, 181 insertions(+), 67 deletions(-) create mode 100644 tests/Fixtures/Fruit/AnimalFruit.php diff --git a/src/RuleBuilders/Architecture/Architecture.php b/src/RuleBuilders/Architecture/Architecture.php index ab23c935..834ef433 100644 --- a/src/RuleBuilders/Architecture/Architecture.php +++ b/src/RuleBuilders/Architecture/Architecture.php @@ -3,11 +3,9 @@ namespace Arkitect\RuleBuilders\Architecture; -use Arkitect\Expression\Boolean\Andx; -use Arkitect\Expression\Boolean\Not; +use Arkitect\Expression\Boolean\Orx; use Arkitect\Expression\Expression; -use Arkitect\Expression\ForClasses\DependsOnlyOnTheseNamespaces; -use Arkitect\Expression\ForClasses\NotDependsOnTheseNamespaces; +use Arkitect\Expression\ForClasses\DependsOnlyOnTheseExpressions; use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces; use Arkitect\Rules\Rule; @@ -15,19 +13,19 @@ class Architecture implements Component, DefinedBy, Where, MayDependOnComponents { /** @var string */ private $componentName; - /** @var array */ + /** @var array */ private $componentSelectors; /** @var array */ private $allowedDependencies; /** @var array */ - private $componentDependsOnlyOnTheseNamespaces; + private $componentDependsOnlyOnTheseComponents; private function __construct() { $this->componentName = ''; $this->componentSelectors = []; $this->allowedDependencies = []; - $this->componentDependsOnlyOnTheseNamespaces = []; + $this->componentDependsOnlyOnTheseComponents = []; } public static function withComponents(): Component @@ -72,7 +70,7 @@ public function shouldNotDependOnAnyComponent() public function shouldOnlyDependOnComponents(string ...$componentNames) { - $this->componentDependsOnlyOnTheseNamespaces[$this->componentName] = $componentNames; + $this->componentDependsOnlyOnTheseComponents[$this->componentName] = $componentNames; return $this; } @@ -93,63 +91,61 @@ public function mayDependOnAnyComponent() public function rules(): iterable { - $layerNames = array_keys($this->componentSelectors); - foreach ($this->componentSelectors as $name => $selector) { if (isset($this->allowedDependencies[$name])) { - $forbiddenComponents = array_diff($layerNames, [$name], $this->allowedDependencies[$name]); - - if (!empty($forbiddenComponents)) { - yield Rule::allClasses() - ->that(\is_string($selector) ? new ResideInOneOfTheseNamespaces($selector) : $selector) - ->should($this->createForbiddenExpression($forbiddenComponents)) - ->because('of component architecture'); - } + yield Rule::allClasses() + ->that(\is_string($selector) ? new ResideInOneOfTheseNamespaces($selector) : $selector) + ->should($this->createAllowedExpression( + array_merge([$name], $this->allowedDependencies[$name]) + )) + ->because('of component architecture'); } - if (!isset($this->componentDependsOnlyOnTheseNamespaces[$name])) { - continue; + if (isset($this->componentDependsOnlyOnTheseComponents[$name])) { + yield Rule::allClasses() + ->that(\is_string($selector) ? new ResideInOneOfTheseNamespaces($selector) : $selector) + ->should($this->createAllowedExpression($this->componentDependsOnlyOnTheseComponents[$name])) + ->because('of component architecture'); } + } + } - $allowedDependencies = array_map(function (string $componentName): string { - return $this->componentSelectors[$componentName]; - }, $this->componentDependsOnlyOnTheseNamespaces[$name]); + private function createAllowedExpression(array $components): Expression + { + $namespaceSelectors = $this->extractComponentsNamespaceSelectors($components); + + $expressionSelectors = $this->extractComponentExpressionSelectors($components); + + if ([] === $namespaceSelectors && [] === $expressionSelectors) { + return new Orx(); // always true + } - yield Rule::allClasses() - ->that(new ResideInOneOfTheseNamespaces($selector)) - ->should(new DependsOnlyOnTheseNamespaces(...$allowedDependencies)) - ->because('of component architecture'); + if ([] !== $namespaceSelectors) { + $expressionSelectors[] = new ResideInOneOfTheseNamespaces(...$namespaceSelectors); } + + return new DependsOnlyOnTheseExpressions(...$expressionSelectors); } - public function createForbiddenExpression(array $forbiddenComponents): Expression + private function extractComponentsNamespaceSelectors(array $components): array { - $forbiddenNamespaceSelectors = array_filter( + return array_filter( array_map(function (string $componentName): ?string { $selector = $this->componentSelectors[$componentName]; return \is_string($selector) ? $selector : null; - }, $forbiddenComponents) + }, $components) ); + } - $forbiddenExpressionSelectors = array_filter( + private function extractComponentExpressionSelectors(array $components): array + { + return array_filter( array_map(function (string $componentName): ?Expression { $selector = $this->componentSelectors[$componentName]; return \is_string($selector) ? null : $selector; - }, $forbiddenComponents) + }, $components) ); - - $forbiddenExpressionList = []; - if ([] !== $forbiddenNamespaceSelectors) { - $forbiddenExpressionList[] = new NotDependsOnTheseNamespaces(...$forbiddenNamespaceSelectors); - } - if ([] !== $forbiddenExpressionSelectors) { - $forbiddenExpressionList[] = new Not(new Andx(...$forbiddenExpressionSelectors)); - } - - return 1 === \count($forbiddenExpressionList) - ? array_pop($forbiddenExpressionList) - : new Andx(...$forbiddenExpressionList); } } diff --git a/tests/Fixtures/Fruit/AnimalFruit.php b/tests/Fixtures/Fruit/AnimalFruit.php new file mode 100644 index 00000000..d2918add --- /dev/null +++ b/tests/Fixtures/Fruit/AnimalFruit.php @@ -0,0 +1,20 @@ +cat = $cat; + } +} diff --git a/tests/Unit/Architecture/ArchitectureTest.php b/tests/Unit/Architecture/ArchitectureTest.php index 4c810be4..9dcb3720 100644 --- a/tests/Unit/Architecture/ArchitectureTest.php +++ b/tests/Unit/Architecture/ArchitectureTest.php @@ -3,10 +3,7 @@ namespace Arkitect\Tests\Unit\Architecture; -use Arkitect\Expression\Boolean\Andx; -use Arkitect\Expression\Boolean\Not; -use Arkitect\Expression\ForClasses\DependsOnlyOnTheseNamespaces; -use Arkitect\Expression\ForClasses\NotDependsOnTheseNamespaces; +use Arkitect\Expression\ForClasses\DependsOnlyOnTheseExpressions; use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces; use Arkitect\RuleBuilders\Architecture\Architecture; use Arkitect\Rules\Rule; @@ -30,15 +27,26 @@ public function test_layered_architecture(): void $expectedRules = [ Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) - ->should(new NotDependsOnTheseNamespaces('App\*\Application\*', 'App\*\Infrastructure\*')) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Domain\*') + )) ->because('of component architecture'), Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Application\*')) - ->should(new NotDependsOnTheseNamespaces('App\*\Infrastructure\*')) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Application\*', 'App\*\Domain\*') + )) + ->because('of component architecture'), + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*')) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*', 'App\*\Domain\*', 'App\*\Application\*') + )) ->because('of component architecture'), ]; - self::assertEquals($expectedRules, iterator_to_array($rules)); + $actualRules = iterator_to_array($rules); + self::assertEquals($expectedRules, $actualRules); } public function test_layered_architecture_with_expression(): void @@ -58,20 +66,26 @@ public function test_layered_architecture_with_expression(): void $expectedRules = [ Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) - ->should(new Not(new Andx( - new ResideInOneOfTheseNamespaces('App\*\Application\*'), - new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*') - ))) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Domain\*') + )) ->because('of component architecture'), Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Application\*')) - ->should(new Not(new Andx( - new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*') - ))) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Application\*', 'App\*\Domain\*') + )) + ->because('of component architecture'), + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*')) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*', 'App\*\Domain\*', 'App\*\Application\*') + )) ->because('of component architecture'), ]; - self::assertEquals($expectedRules, iterator_to_array($rules)); + $actualRules = iterator_to_array($rules); + self::assertEquals($expectedRules, $actualRules); } public function test_layered_architecture_with_mix_of_namespace_and_expression(): void @@ -90,20 +104,26 @@ public function test_layered_architecture_with_mix_of_namespace_and_expression() $expectedRules = [ Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) - ->should(new Andx( - new NotDependsOnTheseNamespaces('App\*\Infrastructure\*'), - new Not(new Andx( - new ResideInOneOfTheseNamespaces('App\*\Application\*') - )) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Domain\*') )) ->because('of component architecture'), Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Application\*')) - ->should(new NotDependsOnTheseNamespaces('App\*\Infrastructure\*')) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Application\*', 'App\*\Domain\*') + )) + ->because('of component architecture'), + Rule::allClasses() + ->that(new ResideInOneOfTheseNamespaces('App\*\Infrastructure\*')) + ->should(new DependsOnlyOnTheseExpressions( + new ResideInOneOfTheseNamespaces('App\*\Domain\*', 'App\*\Application\*', 'App\*\Infrastructure\*') + )) ->because('of component architecture'), ]; - self::assertEquals($expectedRules, iterator_to_array($rules)); + $actualRules = iterator_to_array($rules); + self::assertEquals($expectedRules, $actualRules); } public function test_layered_architecture_with_depends_only_on_components(): void @@ -117,7 +137,7 @@ public function test_layered_architecture_with_depends_only_on_components(): voi $expectedRules = [ Rule::allClasses() ->that(new ResideInOneOfTheseNamespaces('App\*\Domain\*')) - ->should(new DependsOnlyOnTheseNamespaces('App\*\Domain\*')) + ->should(new DependsOnlyOnTheseExpressions(new ResideInOneOfTheseNamespaces('App\*\Domain\*'))) ->because('of component architecture'), ]; diff --git a/tests/Unit/Rules/RuleCheckerTest.php b/tests/Unit/Rules/RuleCheckerTest.php index 0e78786b..06d46de8 100644 --- a/tests/Unit/Rules/RuleCheckerTest.php +++ b/tests/Unit/Rules/RuleCheckerTest.php @@ -4,15 +4,26 @@ namespace Arkitect\Tests\Unit\Rules; use Arkitect\Analyzer\ClassDescription; +use Arkitect\Analyzer\FileParserFactory; use Arkitect\Analyzer\Parser; use Arkitect\ClassSet; use Arkitect\ClassSetRules; use Arkitect\CLI\Progress\VoidProgress; use Arkitect\CLI\Runner; +use Arkitect\CLI\TargetPhpVersion; +use Arkitect\Expression\ForClasses\HaveNameMatching; +use Arkitect\Expression\ForClasses\Implement; +use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces; use Arkitect\Rules\DSL\ArchRule; use Arkitect\Rules\ParsingErrors; +use Arkitect\Rules\Rule; use Arkitect\Rules\Violation; use Arkitect\Rules\Violations; +use Arkitect\Tests\Fixtures\Animal\AnimalInterface; +use Arkitect\Tests\Fixtures\Fruit\AnimalFruit; +use Arkitect\Tests\Fixtures\Fruit\CavendishBanana; +use Arkitect\Tests\Fixtures\Fruit\DwarfCavendishBanana; +use Arkitect\Tests\Fixtures\Fruit\FruitInterface; use PHPUnit\Framework\TestCase; use Symfony\Component\Finder\SplFileInfo; @@ -37,6 +48,73 @@ public function test_should_run_parse_on_all_files_in_class_set(): void self::assertCount(3, $violations); } + + public function test_can_exclude_files_or_directories_from_multiple_dir_class_set_with_no_violations(): void + { + $classSet = ClassSet::fromDir(\FIXTURES_PATH); + + $rules[] = Rule::allClasses() + ->except(FruitInterface::class, CavendishBanana::class, DwarfCavendishBanana::class, AnimalFruit::class) + ->that(new ResideInOneOfTheseNamespaces('Arkitect\Tests\Fixtures\Fruit')) + ->should(new Implement(FruitInterface::class)) + ->because('this tests that string exceptions fail'); + + $rules[] = Rule::allClasses() + ->exceptExpression(new HaveNameMatching('*TestCase')) + ->that(new ResideInOneOfTheseNamespaces('Arkitect\Tests\Fixtures\Animal')) + ->should(new Implement(AnimalInterface::class)) + ->because('this tests that expression exceptions fail'); + + $runner = new Runner(); + + $runner->check( + ClassSetRules::create($classSet, ...$rules), + new VoidProgress(), + FileParserFactory::createFileParser(TargetPhpVersion::create(null)), + $violations = new Violations(), + new ParsingErrors() + ); + + self::assertCount(0, $violations); + } + + public function test_can_exclude_files_or_directories_from_multiple_dir_class_set_with_violations(): void + { + $classSet = ClassSet::fromDir(\FIXTURES_PATH); + + $rules[] = Rule::allClasses() + ->except(FruitInterface::class, CavendishBanana::class, AnimalFruit::class) + ->that(new ResideInOneOfTheseNamespaces('Arkitect\Tests\Fixtures\Fruit')) + ->should(new Implement(FruitInterface::class)) + ->because('this tests that string exceptions fail'); + + $rules[] = Rule::allClasses() + ->exceptExpression(new HaveNameMatching('*NotExistingSoItFails')) + ->that(new ResideInOneOfTheseNamespaces('Arkitect\Tests\Fixtures\Animal')) + ->should(new Implement(AnimalInterface::class)) + ->because('this tests that expression exceptions fail'); + + $runner = new Runner(); + + $runner->check( + ClassSetRules::create($classSet, ...$rules), + new VoidProgress(), + FileParserFactory::createFileParser(TargetPhpVersion::create(null)), + $violations = new Violations(), + new ParsingErrors() + ); + + self::assertCount(2, $violations); + $expectedViolations = "Arkitect\Tests\Fixtures\Animal\CatTestCase has 1 violations + should implement Arkitect\Tests\Fixtures\Animal\AnimalInterface because this tests + that expression exceptions fail Arkitect\Tests\Fixtures\Fruit\DwarfCavendishBanana has 1 violations + should implement Arkitect\Tests\Fixtures\Fruit\FruitInterface because + this tests that string exceptions fail"; + self::assertEquals( + preg_replace('/\s+/', ' ', $expectedViolations), + preg_replace('/\s+/', ' ', trim($violations->toString())) + ); + } } class FakeClassSet extends ClassSet