From cfdf767ed28cdfd224cf646dac07fae1db93c353 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 21 Sep 2026 09:49:30 +0200 Subject: [PATCH] [symfony] Add NoServiceJugglingRule - do not pass an injected service to another own method --- README.md | 49 +++ config/symfony-rules.neon | 1 + src/Enum/RuleIdentifier.php | 2 + src/Rules/Symfony/NoServiceJugglingRule.php | 375 ++++++++++++++++++ .../Fixture/AutowiredJugglingService.php | 23 ++ .../InheritedMethodJugglingService.php | 22 + .../Fixture/PromotedJugglingService.php | 24 ++ .../Fixture/SkippedJugglingService.php | 36 ++ .../Fixture/TraitJugglingService.php | 24 ++ .../VaryingArgumentJugglingService.php | 26 ++ .../NoServiceJugglingRuleTest.php | 53 +++ .../Source/ParentJugglingService.php | 12 + .../Source/SomeHandleTrait.php | 12 + .../Source/SomeUserHelper.php | 13 + 14 files changed, 672 insertions(+) create mode 100644 src/Rules/Symfony/NoServiceJugglingRule.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Fixture/AutowiredJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Fixture/InheritedMethodJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Fixture/PromotedJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Fixture/SkippedJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Fixture/TraitJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Fixture/VaryingArgumentJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/NoServiceJugglingRuleTest.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Source/ParentJugglingService.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Source/SomeHandleTrait.php create mode 100644 tests/Rules/Symfony/NoServiceJugglingRule/Source/SomeUserHelper.php diff --git a/README.md b/README.md index 7106dfe7..898a13b3 100644 --- a/README.md +++ b/README.md @@ -2801,6 +2801,55 @@ public function __construct(
+### NoServiceJugglingRule + +A service injected in `__construct()` or an `autowire*()` method must not be passed to another method of the same class. The called method has its own constructor, so it can take the service directly instead of receiving it through a parameter list. + +```yaml +rules: + - Symplify\PHPStanRules\Rules\Symfony\NoServiceJugglingRule +``` + +```php +public function run(): void +{ + $this->handle($this->userHelper); +} + +public function handle(UserHelper $userHelper): void +{ +} +``` + +:x: + +
+ +```php +public function __construct( + private readonly UserHelper $userHelper, +) { +} + +public function run(): void +{ + $this->handle(); +} + +public function handle(): void +{ + // use $this->userHelper directly +} +``` + +:+1: + +
+ +--- + +
+ ## 4. PHPUnit-specific Rules ### NoAssertFuncCallInTestsRule diff --git a/config/symfony-rules.neon b/config/symfony-rules.neon index 76b6fdf1..0be3320d 100644 --- a/config/symfony-rules.neon +++ b/config/symfony-rules.neon @@ -17,6 +17,7 @@ rules: - Symplify\PHPStanRules\Rules\Symfony\RequireRouteNameToGenerateControllerRouteRule # dependency injection + - Symplify\PHPStanRules\Rules\Symfony\NoServiceJugglingRule - Symplify\PHPStanRules\Rules\Symfony\NoGetInControllerRule - Symplify\PHPStanRules\Rules\Symfony\NoGetInCommandRule - Symplify\PHPStanRules\Rules\Symfony\NoGetDoctrineInControllerRule diff --git a/src/Enum/RuleIdentifier.php b/src/Enum/RuleIdentifier.php index 41cdf2fb..2ac9c714 100644 --- a/src/Enum/RuleIdentifier.php +++ b/src/Enum/RuleIdentifier.php @@ -93,4 +93,6 @@ final class RuleIdentifier public const string NO_PROPERTY_TO_PROPERTY_ASSIGN = 'symplify.noPropertyToPropertyAssign'; public const string REQUIRE_ARRAY_SHAPE_RETURN = 'symplify.requireArrayShapeReturn'; + + public const string NO_SERVICE_JUGGLING = 'symplify.noServiceJuggling'; } diff --git a/src/Rules/Symfony/NoServiceJugglingRule.php b/src/Rules/Symfony/NoServiceJugglingRule.php new file mode 100644 index 00000000..376c43d3 --- /dev/null +++ b/src/Rules/Symfony/NoServiceJugglingRule.php @@ -0,0 +1,375 @@ + + */ +final class NoServiceJugglingRule implements Rule +{ + public const string ERROR_MESSAGE = 'Service "$this->%s" is passed to "%s()" as an argument. Inject "%s" in the constructor of the class that uses it instead'; + + private const string CONSTRUCTOR_NAME = '__construct'; + + private const string AUTOWIRE_PREFIX = 'autowire'; + + private const string REQUIRED_ATTRIBUTE = Required::class; + + public function getNodeType(): string + { + return InClassNode::class; + } + + /** + * @param InClassNode $node + * + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + $classLike = $node->getOriginalNode(); + if (! $classLike instanceof Class_) { + return []; + } + + $classReflection = $node->getClassReflection(); + + $injectedServiceTypes = $this->resolveInjectedServiceTypes($classLike); + if ($injectedServiceTypes === []) { + return []; + } + + $ownMethodCalls = $this->resolveOwnMethodCalls($classLike, $classReflection); + if ($ownMethodCalls === []) { + return []; + } + + $constantArgumentKeys = $this->resolveConstantArgumentKeys($ownMethodCalls); + + $ruleErrors = []; + + foreach ($ownMethodCalls as [$call, $calledMethodName]) { + foreach ($call->getArgs() as $position => $arg) { + $propertyName = $this->matchThisPropertyName($arg->value); + if ($propertyName === null || ! isset($injectedServiceTypes[$propertyName])) { + continue; + } + + if (! isset($constantArgumentKeys[$this->createArgumentKey($call, $calledMethodName, $arg, $position)])) { + continue; + } + + $ruleErrors[] = RuleErrorBuilder::message(sprintf( + self::ERROR_MESSAGE, + $propertyName, + $calledMethodName, + $injectedServiceTypes[$propertyName] + )) + ->identifier(RuleIdentifier::NO_SERVICE_JUGGLING) + ->line($arg->getStartLine()) + ->build(); + } + } + + return $ruleErrors; + } + + /** + * Properties filled by __construct() or an autowire*() method, e.g. "userHelper" => "App\UserHelper". + * + * @return array + */ + private function resolveInjectedServiceTypes(Class_ $class): array + { + $injectedServiceTypes = []; + + foreach ($class->getMethods() as $classMethod) { + if (! $this->isInjectingMethod($classMethod)) { + continue; + } + + $paramTypes = []; + + foreach ($classMethod->params as $param) { + $className = $this->matchObjectTypeName($param); + if ($className === null) { + continue; + } + + if (! $param->var instanceof Variable || ! is_string($param->var->name)) { + continue; + } + + $paramTypes[$param->var->name] = $className; + + // promoted property keeps the param name + if ($param->flags !== 0) { + $injectedServiceTypes[$param->var->name] = $className; + } + } + + foreach ($this->resolveAssignedProperties($classMethod) as $propertyName => $variableName) { + if (isset($paramTypes[$variableName])) { + $injectedServiceTypes[$propertyName] = $paramTypes[$variableName]; + } + } + } + + return $injectedServiceTypes; + } + + private function isInjectingMethod(ClassMethod $classMethod): bool + { + $methodName = $classMethod->name->toString(); + + if ($classMethod->name->toLowerString() === self::CONSTRUCTOR_NAME) { + return true; + } + + if (str_starts_with($methodName, self::AUTOWIRE_PREFIX)) { + return true; + } + + foreach ($classMethod->attrGroups as $attrGroup) { + foreach ($attrGroup->attrs as $attr) { + if ($attr->name->toString() === self::REQUIRED_ATTRIBUTE) { + return true; + } + } + } + + return false; + } + + /** + * The "$this->userHelper = $userHelper;" assignments, as "userHelper" => "userHelper". + * + * @return array + */ + private function resolveAssignedProperties(ClassMethod $classMethod): array + { + $assignedProperties = []; + + $nodeFinder = new NodeFinder(); + + /** @var Assign[] $assigns */ + $assigns = $nodeFinder->findInstanceOf((array) $classMethod->stmts, Assign::class); + + foreach ($assigns as $assign) { + $propertyName = $this->matchThisPropertyName($assign->var); + if ($propertyName === null) { + continue; + } + + if (! $assign->expr instanceof Variable || ! is_string($assign->expr->name)) { + continue; + } + + $assignedProperties[$propertyName] = $assign->expr->name; + } + + return $assignedProperties; + } + + /** + * The calls that stay inside the class hierarchy: "$this->someMethod()" and "parent::someMethod()". + * + * @return list + */ + private function resolveLocalCalls(ClassMethod $classMethod): array + { + $nodeFinder = new NodeFinder(); + + /** @var array $calls */ + $calls = $nodeFinder->find((array) $classMethod->stmts, static function (Node $node): bool { + if ($node instanceof MethodCall) { + return $node->var instanceof Variable && $node->var->name === 'this'; + } + + if ($node instanceof StaticCall) { + return $node->class instanceof Name && $node->class->toLowerString() === 'parent'; + } + + return false; + }); + + return array_values($calls); + } + + /** + * The calls to a method the class itself owns, so the dependency can be moved into a constructor. + * + * @return list + */ + private function resolveOwnMethodCalls(Class_ $class, ClassReflection $classReflection): array + { + $ownMethodCalls = []; + + foreach ($class->getMethods() as $classMethod) { + foreach ($this->resolveLocalCalls($classMethod) as $call) { + // "$this->toText(...)" has no arguments to look at + if ($call->isFirstClassCallable()) { + continue; + } + + $calledMethodName = $call->name instanceof Identifier ? $call->name->toString() : null; + if ($calledMethodName === null) { + continue; + } + + if (! $this->isOwnMethod($call, $class, $classReflection, $calledMethodName)) { + continue; + } + + $ownMethodCalls[] = [$call, $calledMethodName]; + } + } + + return $ownMethodCalls; + } + + /** + * A method the class can change: one declared right here, or the one of the parent an explicit "parent::" targets. + * + * An inherited method reached through "$this->" is left out: it is shared by every child, so the service is an + * argument of the call, not a dependency of the method. The same goes for a method brought in by a trait, as a + * trait has no constructor. + * + * @param MethodCall|StaticCall $call + */ + private function isOwnMethod(Node $call, Class_ $class, ClassReflection $classReflection, string $methodName): bool + { + if ($call instanceof StaticCall) { + return ! $this->isDeclaredInTrait($classReflection->getParentClass(), $methodName); + } + + return $class->getMethod($methodName) instanceof ClassMethod; + } + + /** + * The argument positions that always get the very same value, as only those stand for a fixed dependency. + * A position filled differently by another call is a parameter of its own. + * + * @param list $ownMethodCalls + * + * @return array + */ + private function resolveConstantArgumentKeys(array $ownMethodCalls): array + { + $argumentValues = []; + + foreach ($ownMethodCalls as [$call, $calledMethodName]) { + foreach ($call->getArgs() as $position => $arg) { + $argumentKey = $this->createArgumentKey($call, $calledMethodName, $arg, $position); + + $argumentValues[$argumentKey][$this->matchThisPropertyName($arg->value) ?? '#other'] = true; + } + } + + $constantArgumentKeys = []; + + foreach ($argumentValues as $argumentKey => $values) { + if (count($values) === 1) { + $constantArgumentKeys[$argumentKey] = true; + } + } + + return $constantArgumentKeys; + } + + /** + * @param MethodCall|StaticCall $call + */ + private function createArgumentKey(Node $call, string $calledMethodName, Arg $arg, int $position): string + { + $callKind = $call instanceof StaticCall ? 'parent' : 'this'; + $argName = $arg->name instanceof Identifier ? $arg->name->toString() : (string) $position; + + return $callKind . '::' . $calledMethodName . '#' . $argName; + } + + /** + * A trait has no constructor to inject into, so its methods can only take the service as a parameter. + */ + private function isDeclaredInTrait(?ClassReflection $classReflection, string $methodName): bool + { + if (! $classReflection instanceof ClassReflection) { + return false; + } + + return array_any( + $classReflection->getTraits(true), + fn (ClassReflection $classReflection): bool => $classReflection->hasNativeMethod($methodName) + ); + } + + /** + * The property name of "$this->userHelper", null for anything else. + */ + private function matchThisPropertyName(Node $node): ?string + { + if (! $node instanceof PropertyFetch) { + return null; + } + + if (! $node->var instanceof Variable || $node->var->name !== 'this') { + return null; + } + + if (! $node->name instanceof Identifier) { + return null; + } + + return $node->name->toString(); + } + + /** + * Only class types count, a scalar or an array is a value, not a service. + */ + private function matchObjectTypeName(Param $param): ?string + { + $type = $param->type; + if ($type instanceof NullableType) { + $type = $type->type; + } + + if (! $type instanceof Name) { + return null; + } + + return $type->toString(); + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/AutowiredJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/AutowiredJugglingService.php new file mode 100644 index 00000000..f3b947c9 --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/AutowiredJugglingService.php @@ -0,0 +1,23 @@ +userHelper = $userHelper; + } + + public function run(): void + { + parent::handle($this->userHelper); + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/InheritedMethodJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/InheritedMethodJugglingService.php new file mode 100644 index 00000000..14542a25 --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/InheritedMethodJugglingService.php @@ -0,0 +1,22 @@ +handle($this->userHelper); + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/PromotedJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/PromotedJugglingService.php new file mode 100644 index 00000000..4f24d06f --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/PromotedJugglingService.php @@ -0,0 +1,24 @@ +handle($this->userHelper); + } + + public function handle(SomeUserHelper $userHelper): void + { + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/SkippedJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/SkippedJugglingService.php new file mode 100644 index 00000000..cca424f6 --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/SkippedJugglingService.php @@ -0,0 +1,36 @@ +handle($this->secret); + + // a param of its own, the service is not taken from the class + $this->handleService($localUserHelper); + + // another service is the one being called, not a method of this class + $this->userHelper->getUser(); + } + + public function handle(string $secret): void + { + } + + public function handleService(SomeUserHelper $userHelper): void + { + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/TraitJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/TraitJugglingService.php new file mode 100644 index 00000000..c4112f7a --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/TraitJugglingService.php @@ -0,0 +1,24 @@ +handleInTrait($this->userHelper); + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/VaryingArgumentJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/VaryingArgumentJugglingService.php new file mode 100644 index 00000000..291e882b --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Fixture/VaryingArgumentJugglingService.php @@ -0,0 +1,26 @@ +handle($this->userHelper); + $this->handle($otherUserHelper); + } + + public function handle(SomeUserHelper $userHelper): void + { + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/NoServiceJugglingRuleTest.php b/tests/Rules/Symfony/NoServiceJugglingRule/NoServiceJugglingRuleTest.php new file mode 100644 index 00000000..6fa14226 --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/NoServiceJugglingRuleTest.php @@ -0,0 +1,53 @@ + + */ +final class NoServiceJugglingRuleTest extends RuleTestCase +{ + /** + * @param list $expectedErrorsWithLines + */ + #[DataProvider('provideData')] + public function testRule(string $filePath, array $expectedErrorsWithLines): void + { + $this->analyse([$filePath], $expectedErrorsWithLines); + } + + /** + * @return Iterator, mixed>> + */ + public static function provideData(): Iterator + { + $errorMessage = sprintf( + NoServiceJugglingRule::ERROR_MESSAGE, + 'userHelper', + 'handle', + SomeUserHelper::class + ); + + yield [__DIR__ . '/Fixture/PromotedJugglingService.php', [[$errorMessage, 18]]]; + yield [__DIR__ . '/Fixture/AutowiredJugglingService.php', [[$errorMessage, 21]]]; + + yield [__DIR__ . '/Fixture/SkippedJugglingService.php', []]; + yield [__DIR__ . '/Fixture/TraitJugglingService.php', []]; + yield [__DIR__ . '/Fixture/VaryingArgumentJugglingService.php', []]; + yield [__DIR__ . '/Fixture/InheritedMethodJugglingService.php', []]; + } + + protected function getRule(): Rule + { + return new NoServiceJugglingRule(); + } +} diff --git a/tests/Rules/Symfony/NoServiceJugglingRule/Source/ParentJugglingService.php b/tests/Rules/Symfony/NoServiceJugglingRule/Source/ParentJugglingService.php new file mode 100644 index 00000000..c43c2522 --- /dev/null +++ b/tests/Rules/Symfony/NoServiceJugglingRule/Source/ParentJugglingService.php @@ -0,0 +1,12 @@ +