Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to check for multiple extends #432

Merged
merged 2 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ $rules[] = Rule::allClasses()
->that(new ResideInOneOfTheseNamespaces('App\Controller'))
->should(new Extend('App\Controller\AbstractController'))
->because('we want to be sure that all controllers extend AbstractController');

You can add multiple parameters, the violation will happen when none of them match
```

### Has an attribute (requires PHP >= 8.0)
Expand Down Expand Up @@ -348,6 +350,8 @@ $rules[] = Rule::allClasses()
->that(new ResideInOneOfTheseNamespaces('App\Controller\Admin'))
->should(new NotExtend('App\Controller\AbstractController'))
->because('we want to be sure that all admin controllers not extend AbstractController for security reasons');

You can add multiple parameters, the violation will happen when one of them match
```

### Don't have dependency outside a namespace
Expand Down
19 changes: 12 additions & 7 deletions src/Expression/ForClasses/Extend.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,30 @@

class Extend implements Expression
{
/** @var string */
private $className;
/** @var string[] */
private $classNames;

public function __construct(string $className)
public function __construct(string ...$classNames)
{
$this->className = $className;
$this->classNames = $classNames;
}

public function describe(ClassDescription $theClass, string $because): Description
{
return new Description("should extend {$this->className}", $because);
$desc = implode(', ', $this->classNames);

return new Description("should extend one of these classes: {$desc}", $because);
}

public function evaluate(ClassDescription $theClass, Violations $violations, string $because): void
{
$extends = $theClass->getExtends();

if (null !== $extends && $extends->matches($this->className)) {
return;
/** @var string $className */
foreach ($this->classNames as $className) {
if (null !== $extends && $extends->matches($className)) {
return;
}
}

$violation = Violation::create(
Expand Down
35 changes: 18 additions & 17 deletions src/Expression/ForClasses/NotExtend.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,36 +13,37 @@

class NotExtend implements Expression
{
/** @var string */
private $className;
/** @var string[] */
private $classNames;

public function __construct(string $className)
public function __construct(string ...$classNames)
{
$this->className = $className;
$this->classNames = $classNames;
}

public function describe(ClassDescription $theClass, string $because): Description
{
return new Description("should not extend {$this->className}", $because);
$desc = implode(', ', $this->classNames);

return new Description("should not extend one of these classes: {$desc}", $because);
}

public function evaluate(ClassDescription $theClass, Violations $violations, string $because): void
{
$extends = $theClass->getExtends();

if (null === $extends) {
return;
}
/** @var string $className */
foreach ($this->classNames as $className) {
if (null !== $extends && $extends->matches($className)) {
$violation = Violation::create(
$theClass->getFQCN(),
ViolationMessage::selfExplanatory($this->describe($theClass, $because))
);

if ($extends->toString() !== $this->className) {
return;
}
$violations->add($violation);

$violation = Violation::create(
$theClass->getFQCN(),
ViolationMessage::selfExplanatory($this->describe($theClass, $because))
);

$violations->add($violation);
return;
}
}
}
}
8 changes: 4 additions & 4 deletions tests/E2E/Cli/DebugExpressionCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,16 @@ public function test_some_classes_found(): void
public function test_meaningful_errors_for_too_few_arguments_for_the_expression(): void
{
$appTester = $this->createAppTester();
$appTester->run(['debug:expression', 'expression' => 'NotExtend', 'arguments' => [], '--from-dir' => __DIR__.'/../_fixtures/mvc/Domain']);
$this->assertEquals("Error: Too few arguments for 'NotExtend'.\n", $appTester->getDisplay());
$appTester->run(['debug:expression', 'expression' => 'NotImplement', 'arguments' => [], '--from-dir' => __DIR__.'/../_fixtures/mvc/Domain']);
$this->assertEquals("Error: Too few arguments for 'NotImplement'.\n", $appTester->getDisplay());
$this->assertEquals(2, $appTester->getStatusCode());
}

public function test_meaningful_errors_for_too_many_arguments_for_the_expression(): void
{
$appTester = $this->createAppTester();
$appTester->run(['debug:expression', 'expression' => 'NotExtend', 'arguments' => ['First', 'Second'], '--from-dir' => __DIR__.'/../_fixtures/mvc/Domain']);
$this->assertEquals("Error: Too many arguments for 'NotExtend'.\n", $appTester->getDisplay());
$appTester->run(['debug:expression', 'expression' => 'NotImplement', 'arguments' => ['First', 'Second'], '--from-dir' => __DIR__.'/../_fixtures/mvc/Domain']);
$this->assertEquals("Error: Too many arguments for 'NotImplement'.\n", $appTester->getDisplay());
$this->assertEquals(2, $appTester->getStatusCode());
}

Expand Down
19 changes: 17 additions & 2 deletions tests/Unit/Expressions/ForClasses/ExtendTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public function test_it_should_return_violation_error_when_class_not_extend(): v
$extend->evaluate($classDescription, $violations, 'we want to add this rule for our software');

self::assertEquals(1, $violations->count());
self::assertEquals('should extend My\BaseClass because we want to add this rule for our software', $violations->get(0)->getError());
self::assertEquals('should extend one of these classes: My\BaseClass because we want to add this rule for our software', $violations->get(0)->getError());
}

public function test_it_should_return_violation_error_if_extend_is_null(): void
Expand All @@ -97,6 +97,21 @@ public function test_it_should_return_violation_error_if_extend_is_null(): void
$extend->evaluate($classDescription, $violations, $because);

self::assertEquals(1, $violations->count());
self::assertEquals('should extend My\BaseClass because we want to add this rule for our software', $violationError);
self::assertEquals('should extend one of these classes: My\BaseClass because we want to add this rule for our software', $violationError);
}

public function test_it_should_accept_multiple_extends(): void
{
$extend = new Extend('My\FirstExtend', 'My\SecondExtend');

$classDescription = (new ClassDescriptionBuilder())
->setClassName('My\Class')
->setExtends('My\SecondExtend', 10)
->build();

$violations = new Violations();
$extend->evaluate($classDescription, $violations, 'because');

self::assertEquals(0, $violations->count());
}
}
28 changes: 27 additions & 1 deletion tests/Unit/Expressions/ForClasses/NotExtendTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function test_it_should_return_violation_error(): void
$notExtend->evaluate($classDescription, $violations, $because);

self::assertEquals(1, $violations->count());
self::assertEquals('should not extend My\BaseClass because we want to add this rule for our software', $violationError);
self::assertEquals('should not extend one of these classes: My\BaseClass because we want to add this rule for our software', $violationError);
}

public function test_it_should_not_return_violation_error_if_extends_another_class(): void
Expand All @@ -62,4 +62,30 @@ public function test_it_should_not_return_violation_error_if_extends_another_cla

self::assertEquals(0, $violations->count());
}

public function test_it_should_return_violation_error_for_multiple_extends(): void
{
$notExtend = new NotExtend('My\FirstExtend', 'My\SecondExtend');

$classDescription = new ClassDescription(
FullyQualifiedClassName::fromString('HappyIsland'),
[],
[],
FullyQualifiedClassName::fromString('My\SecondExtend'),
false,
false,
false,
false,
false,
false
);
$because = 'we want to add this rule for our software';
$violationError = $notExtend->describe($classDescription, $because)->toString();

$violations = new Violations();
$notExtend->evaluate($classDescription, $violations, $because);

self::assertEquals(1, $violations->count());
self::assertEquals('should not extend one of these classes: My\FirstExtend, My\SecondExtend because we want to add this rule for our software', $violationError);
}
}
Loading