From d1b4303b18b41fe607c528abcbf3ec7a86292011 Mon Sep 17 00:00:00 2001 From: Daniel Badura Date: Sat, 18 Jul 2026 11:50:33 +0200 Subject: [PATCH] Rework the rule: now covers child aggregates (even if deprecated, as it was cheap), and now also handles even deeper levels --- docs/getting-started.md | 8 +- extension.neon | 2 + phpstan-baseline.neon | 22 +++- src/DontRecordWhenApplyingExtension.php | 160 ++++++++++++++++++------ tests/Invalid/PersonalInformation.php | 26 ++++ tests/Invalid/Profile.php | 18 +++ tests/Invalid/RecordingHelper.php | 15 +++ tests/Valid/EventCollector.php | 22 ++++ 8 files changed, 228 insertions(+), 45 deletions(-) create mode 100644 tests/Invalid/PersonalInformation.php create mode 100644 tests/Invalid/RecordingHelper.php create mode 100644 tests/Valid/EventCollector.php diff --git a/docs/getting-started.md b/docs/getting-started.md index 8dfb18f..6057f78 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -101,11 +101,11 @@ final class Profile extends BasicAggregateRoot Running PHPStan now produces: ``` -Method Patchlevel\EventSourcing\Aggregate\AggregateRoot::recordThat() is called -from applyProfileCreated which is an apply method. +Method Patchlevel\EventSourcing\Aggregate\BasicAggregateRoot::recordThat() records +an event and is called from apply method applyProfileCreated(). ``` -:::info -The check also follows calls into helper methods, so hiding `recordThat()` behind another method does not bypass the rule. +:::note +The check follows calls into helper methods, even across multiple levels and into traits, so hiding `recordThat()` behind other methods does not bypass the rule. It covers child aggregates as well. Calling a `recordThat()` method on an unrelated object, for example a collaborator service, is not reported. ::: ## Result diff --git a/extension.neon b/extension.neon index f9fee41..44848f2 100644 --- a/extension.neon +++ b/extension.neon @@ -6,5 +6,7 @@ services: - class: Patchlevel\EventSourcingPHPStanExtension\DontRecordWhenApplyingExtension + arguments: + parser: @defaultAnalysisParser tags: - phpstan.restrictedMethodUsageExtension diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6897954..c8b35f1 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1,13 +1,31 @@ parameters: ignoreErrors: - - rawMessage: 'Method Patchlevel\EventSourcing\Aggregate\AggregateRoot::recordThat() is called from applyNameChanged which is an apply method.' + rawMessage: 'Method Patchlevel\EventSourcing\Aggregate\BasicChildAggregate::recordThat() records an event and is called from apply method applyNameChanged().' + identifier: patchlevel.noRecordThatWhenApplying + count: 1 + path: tests/Invalid/PersonalInformation.php + + - + rawMessage: 'Method Patchlevel\EventSourcingPHPStanExtension\Tests\Invalid\Profile::deeplyHiddenRecordThat() records an event and is called from apply method applyNameChanged().' + identifier: patchlevel.noRecordThatWhenApplying + count: 1 + path: tests/Invalid/Profile.php + + - + rawMessage: 'Method Patchlevel\EventSourcingPHPStanExtension\Tests\Invalid\Profile::hiddenRecordThat() records an event and is called from apply method applyNameChanged().' + identifier: patchlevel.noRecordThatWhenApplying + count: 1 + path: tests/Invalid/Profile.php + + - + rawMessage: 'Method Patchlevel\EventSourcingPHPStanExtension\Tests\Invalid\Profile::traitHiddenRecordThat() records an event and is called from apply method applyNameChanged().' identifier: patchlevel.noRecordThatWhenApplying count: 1 path: tests/Invalid/Profile.php - - rawMessage: 'Method Patchlevel\EventSourcing\Aggregate\AggregateRoot::recordThat() is called from applyProfileCreated which is an apply method.' + rawMessage: 'Method Patchlevel\EventSourcing\Aggregate\BasicAggregateRoot::recordThat() records an event and is called from apply method applyProfileCreated().' identifier: patchlevel.noRecordThatWhenApplying count: 1 path: tests/Invalid/Profile.php diff --git a/src/DontRecordWhenApplyingExtension.php b/src/DontRecordWhenApplyingExtension.php index 34b33e3..b30a796 100644 --- a/src/DontRecordWhenApplyingExtension.php +++ b/src/DontRecordWhenApplyingExtension.php @@ -5,41 +5,49 @@ namespace Patchlevel\EventSourcingPHPStanExtension; use Patchlevel\EventSourcing\Aggregate\AggregateRoot; +use Patchlevel\EventSourcing\Aggregate\ChildAggregate; use Patchlevel\EventSourcing\Attribute\Apply; +use PhpParser\Node; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\Expr\Variable; use PhpParser\Node\Identifier; +use PhpParser\Node\Name; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\NodeFinder; -use PhpParser\ParserFactory; use PHPStan\Analyser\Scope; +use PHPStan\Parser\Parser; +use PHPStan\Reflection\ClassReflection; use PHPStan\Reflection\ExtendedMethodReflection; use PHPStan\Reflection\FunctionReflection; use PHPStan\Reflection\MethodReflection; use PHPStan\Rules\RestrictedUsage\RestrictedMethodUsageExtension; use PHPStan\Rules\RestrictedUsage\RestrictedUsage; -use function file_get_contents; +use function array_key_exists; +use function in_array; use function sprintf; +use function strtolower; final class DontRecordWhenApplyingExtension implements RestrictedMethodUsageExtension { private NodeFinder $nodeFinder; - private ParserFactory $parserFactory; /** @var array */ - private array $recordThatCallCache = []; + private array $recordsEventCache = []; - public function __construct() + public function __construct(private readonly Parser $parser) { $this->nodeFinder = new NodeFinder(); - $this->parserFactory = new ParserFactory(); } public function isRestrictedMethodUsage( ExtendedMethodReflection $methodReflection, Scope $scope, ): RestrictedUsage|null { - if (!$methodReflection->getDeclaringClass()->implementsInterface(AggregateRoot::class)) { + $declaringClass = $methodReflection->getDeclaringClass(); + + if (!$this->isAggregate($declaringClass)) { return null; } @@ -53,20 +61,29 @@ public function isRestrictedMethodUsage( return null; } - if (!$this->callsRecordThat($methodReflection)) { + $visited = []; + + if (!$this->recordsEvent($declaringClass, $methodReflection->getName(), $visited)) { return null; } return RestrictedUsage::create( errorMessage: sprintf( - 'Method %s::recordThat() is called from %s which is an apply method.', - AggregateRoot::class, - $function->getName() ?? 'unknown', + 'Method %s::%s() records an event and is called from apply method %s().', + $declaringClass->getName(), + $methodReflection->getName(), + $function->getName(), ), identifier: 'patchlevel.noRecordThatWhenApplying', ); } + private function isAggregate(ClassReflection $classReflection): bool + { + return $classReflection->implementsInterface(AggregateRoot::class) + || $classReflection->implementsInterface(ChildAggregate::class); + } + private function isApplyMethod(FunctionReflection|MethodReflection $function): bool { foreach ($function->getAttributes() as $attribute) { @@ -78,55 +95,120 @@ private function isApplyMethod(FunctionReflection|MethodReflection $function): b return false; } - private function callsRecordThat(ExtendedMethodReflection $methodReflection): bool + /** @param array $visited */ + private function recordsEvent(ClassReflection $classReflection, string $methodName, array &$visited): bool { - if ($methodReflection->getName() === 'recordThat') { + if ($methodName === 'recordThat') { return true; } - $declaringClass = $methodReflection->getDeclaringClass(); - $fileName = $declaringClass->getFileName(); + $cacheKey = sprintf('%s::%s', $classReflection->getName(), $methodName); - if ($fileName === null || $fileName === false) { + if (array_key_exists($cacheKey, $this->recordsEventCache)) { + return $this->recordsEventCache[$cacheKey]; + } + + if (array_key_exists($cacheKey, $visited)) { return false; } - $cacheKey = sprintf('%s::%s', $declaringClass->getName(), $methodReflection->getName()); + $visited[$cacheKey] = true; + + $methodNode = $this->findMethodNode($classReflection, $methodName); + $result = false; - if (isset($this->recordThatCallCache[$cacheKey])) { - return $this->recordThatCallCache[$cacheKey]; + if ($methodNode?->stmts !== null) { + foreach ($this->calledMethodNames($methodNode) as $calledMethodName) { + if ($this->recordsEvent($classReflection, $calledMethodName, $visited)) { + $result = true; + break; + } + } } - $parser = $this->parserFactory->createForNewestSupportedVersion(); - $ast = $parser->parse((string)file_get_contents($fileName)); + $this->recordsEventCache[$cacheKey] = $result; - if ($ast === null) { - $this->recordThatCallCache[$cacheKey] = false; + return $result; + } - return false; + private function findMethodNode(ClassReflection $classReflection, string $methodName): ClassMethod|null + { + $nativeClass = $classReflection->getNativeReflection(); + + if (!$nativeClass->hasMethod($methodName)) { + return null; } - $methodNode = $this->nodeFinder->findFirst( - $ast, - static fn (mixed $node): bool => $node instanceof ClassMethod - && $node->name->toString() === $methodReflection->getName(), + $nativeMethod = $nativeClass->getMethod($methodName); + $fileName = $nativeMethod->getFileName(); + + if ($fileName === false) { + return null; + } + + $candidates = $this->nodeFinder->find( + $this->parser->parseFile($fileName), + static fn (Node $node): bool => $node instanceof ClassMethod + && $node->name->toString() === $nativeMethod->getName(), ); - if (!$methodNode instanceof ClassMethod || $methodNode->stmts === null) { - $this->recordThatCallCache[$cacheKey] = false; + if ($candidates === []) { + return null; + } - return false; + foreach ($candidates as $candidate) { + if ($candidate->getStartLine() === $nativeMethod->getStartLine()) { + return $candidate instanceof ClassMethod ? $candidate : null; + } } - $result = $this->nodeFinder->findFirst( - $methodNode->stmts, - static fn (mixed $node): bool => $node instanceof MethodCall - && $node->name instanceof Identifier - && $node->name->toString() === 'recordThat', - ) !== null; + $candidate = $candidates[0]; - $this->recordThatCallCache[$cacheKey] = $result; + return $candidate instanceof ClassMethod ? $candidate : null; + } - return $result; + /** + * Collects the names of all methods the given method calls on its own + * instance ($this->, self::, static::, parent::). Calls on other objects + * are ignored, their receiver type cannot be resolved without a scope. + * + * @return list + */ + private function calledMethodNames(ClassMethod $methodNode): array + { + $calls = $this->nodeFinder->find( + $methodNode->stmts ?? [], + static function (Node $node): bool { + if ($node instanceof MethodCall) { + return $node->var instanceof Variable + && $node->var->name === 'this' + && $node->name instanceof Identifier; + } + + if ($node instanceof StaticCall) { + return $node->class instanceof Name + && in_array(strtolower($node->class->toString()), ['self', 'static', 'parent'], true) + && $node->name instanceof Identifier; + } + + return false; + }, + ); + + $names = []; + + foreach ($calls as $call) { + if (!$call instanceof MethodCall && !$call instanceof StaticCall) { + continue; + } + + if (!$call->name instanceof Identifier) { + continue; + } + + $names[] = $call->name->toString(); + } + + return $names; } } diff --git a/tests/Invalid/PersonalInformation.php b/tests/Invalid/PersonalInformation.php new file mode 100644 index 0000000..7cebe4d --- /dev/null +++ b/tests/Invalid/PersonalInformation.php @@ -0,0 +1,26 @@ +name = $event->name; + $this->recordThat(new NameChanged($event->name)); + } + + public function name(): string + { + return $this->name; + } +} diff --git a/tests/Invalid/Profile.php b/tests/Invalid/Profile.php index 9ce6ca4..afe8432 100644 --- a/tests/Invalid/Profile.php +++ b/tests/Invalid/Profile.php @@ -6,14 +6,18 @@ use Patchlevel\EventSourcing\Aggregate\Uuid; use Patchlevel\EventSourcing\Attribute\Apply; use Patchlevel\EventSourcing\Attribute\Id; +use Patchlevel\EventSourcingPHPStanExtension\Tests\Valid\EventCollector; use Patchlevel\EventSourcingPHPStanExtension\Tests\Valid\NameChanged; use Patchlevel\EventSourcingPHPStanExtension\Tests\Valid\ProfileCreated; class Profile extends BasicAggregateRoot { + use RecordingHelper; + #[Id] private Uuid $id; private string $name; + private EventCollector $collector; public static function create(Uuid $id, string $name): self { @@ -28,6 +32,7 @@ protected function applyProfileCreated(ProfileCreated $event): void { $this->id = $event->id; $this->name = $event->name; + $this->collector = new EventCollector(); $this->recordThat(new ProfileCreated($event->id, $event->name)); } @@ -36,6 +41,9 @@ protected function applyNameChanged(NameChanged $event): void { $this->name = $event->name; $this->hiddenRecordThat($event); + $this->deeplyHiddenRecordThat($event); + $this->traitHiddenRecordThat($event); + $this->collectEvent($event); } public function hiddenRecordThat(NameChanged $event): void @@ -43,6 +51,16 @@ public function hiddenRecordThat(NameChanged $event): void $this->recordThat(new NameChanged($event->name)); } + public function deeplyHiddenRecordThat(NameChanged $event): void + { + $this->hiddenRecordThat($event); + } + + public function collectEvent(NameChanged $event): void + { + $this->collector->recordThat($event); + } + public function id(): Uuid { return $this->id; diff --git a/tests/Invalid/RecordingHelper.php b/tests/Invalid/RecordingHelper.php new file mode 100644 index 0000000..fd56917 --- /dev/null +++ b/tests/Invalid/RecordingHelper.php @@ -0,0 +1,15 @@ +recordThat(new NameChanged($event->name)); + } +} diff --git a/tests/Valid/EventCollector.php b/tests/Valid/EventCollector.php new file mode 100644 index 0000000..aa2a81c --- /dev/null +++ b/tests/Valid/EventCollector.php @@ -0,0 +1,22 @@ + */ + private array $events = []; + + public function recordThat(object $event): void + { + $this->events[] = $event; + } + + /** @return list */ + public function events(): array + { + return $this->events; + } +}