From 05c7a6a00416cd607a2ce9bce2f5d2eb3c9a1ab0 Mon Sep 17 00:00:00 2001 From: mscherer Date: Sun, 12 Oct 2025 11:59:16 +0200 Subject: [PATCH 1/6] Rule to enforce that certain controller methods must be returned --- .github/workflows/ci.yml | 4 +- README.md | 37 +++++- composer.json | 4 +- phpcs.xml | 13 ++- rules.neon | 6 + ...tionTableMixinClassReflectionExtension.php | 10 +- src/Method/DummyParameter.php | 2 +- .../TableFindByPropertyMethodReflection.php | 6 +- ...leAssociationTypeNodeResolverExtension.php | 2 +- .../ControllerMethodMustReturnRule.php | 106 ++++++++++++++++++ src/Rule/LoadObjectExistsCakeClassRule.php | 2 +- .../AddAssociationMatchOptionsTypesRule.php | 14 +-- ...rmSelectQueryFindMatchOptionsTypesRule.php | 12 +- src/Traits/BaseCakeRegistryReturnTrait.php | 2 +- ...seTraitExpressionTypeResolverExtension.php | 2 +- ...erFetchTableDynamicReturnTypeExtension.php | 6 +- ...sitoryEntityDynamicReturnTypeExtension.php | 2 +- ...sitoryFirstArgIsTheReturnTypeExtension.php | 2 +- ...TableLocatorDynamicReturnTypeExtension.php | 4 +- .../ControllerMethodMustReturnRuleTest.php | 62 ++++++++++ ...ddAssociationMatchOptionsTypesRuleTest.php | 2 +- .../Fake/FailingOrmFindRuleItemsLogic.php | 20 ++-- .../Rule/Model/Fake/FailingRuleItemsTable.php | 12 +- .../Fake/FailingTableGetRuleItemsLogic.php | 4 +- ...lectQueryFindMatchOptionsTypesRuleTest.php | 2 +- .../TableGetMatchOptionsTypesRuleTest.php | 2 +- .../FailingControllerMethodReturnLogic.php | 77 +++++++++++++ tests/test_app/Model/Table/NotesTable.php | 4 +- .../Table/VeryCustomize00009ArticlesTable.php | 2 +- 29 files changed, 352 insertions(+), 71 deletions(-) create mode 100644 src/Rule/Controller/ControllerMethodMustReturnRule.php create mode 100644 tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php create mode 100644 tests/test_app/Controller/FailingControllerMethodReturnLogic.php diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4686feb..8b45c7a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,7 +19,7 @@ jobs: strategy: fail-fast: false matrix: - php-version: ['8.1', '8.2', '8.3'] + php-version: ['8.1', '8.4'] dependencies: ['highest'] include: - php-version: '8.1' @@ -68,7 +68,7 @@ jobs: uses: ramsey/composer-install@v2 - name: Run phpcs - run: vendor/bin/phpcs --report=checkstyle src/ tests/ | cs2pr + run: vendor/bin/phpcs --report=checkstyle | cs2pr - name: Run phpstan if: always() diff --git a/README.md b/README.md index 5dbf0aa..02e1cf4 100644 --- a/README.md +++ b/README.md @@ -148,13 +148,40 @@ This rule check if the options (args) passed to Table::find and SelectQuery are ### TableGetMatchOptionsTypesRule This rule check if the options (args) passed to Table::get are valid find options types. -To enable this rule update your phpstan.neon with: +### ControllerMethodMustReturnRule +This rule enforces that controller methods like `render()` and `redirect()` must be returned to prevent unreachable code. These methods should always be used with a `return` statement to make the control flow explicit. +
+ Examples: + +```php +// Bad - code after render() is unreachable +public function myAction() +{ + $this->render('edit'); + $this->set('data', 'value'); // This will never execute +} + +// Good - explicit return prevents confusion +public function myAction() +{ + return $this->render('edit'); +} + +// Bad - code after redirect() is unreachable +public function myAction() +{ + $this->redirect(['action' => 'index']); + $this->Flash->success('Done'); // This will never execute +} + +// Good - explicit return prevents confusion +public function myAction() +{ + return $this->redirect(['action' => 'index']); +} ``` -parameters: - cakeDC: - disallowEntityArrayAccessRule: true -``` +
### How to disable a rule Each rule has a parameter in cakeDC 'namespace' to enable or disable, it is the same name of the diff --git a/composer.json b/composer.json index 6c6acef..4bcf8b0 100644 --- a/composer.json +++ b/composer.json @@ -42,8 +42,8 @@ } }, "scripts": { - "cs-check": "phpcs -p src/ tests", - "cs-fix": "phpcbf -p src/ tests", + "cs-check": "phpcs -p", + "cs-fix": "phpcbf -p", "test": "phpunit --stderr", "stan-integration": [ "phpstan analyse --debug -c phpstan-test-app.neon", diff --git a/phpcs.xml b/phpcs.xml index 1ae027b..ffbe1e6 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -1,8 +1,11 @@ - - + + - + - - \ No newline at end of file + src/ + tests/ + + /tests/data/ + diff --git a/rules.neon b/rules.neon index 30f5bba..901db64 100644 --- a/rules.neon +++ b/rules.neon @@ -8,6 +8,7 @@ parameters: disallowEntityArrayAccessRule: false getMailerExistsClassRule: true loadComponentExistsClassRule: true + controllerMethodMustReturnRule: true parametersSchema: cakeDC: structure([ addAssociationExistsTableClassRule: anyOf(bool(), arrayOf(bool())) @@ -18,6 +19,7 @@ parametersSchema: disallowEntityArrayAccessRule: anyOf(bool(), arrayOf(bool())) getMailerExistsClassRule: anyOf(bool(), arrayOf(bool())) loadComponentExistsClassRule: anyOf(bool(), arrayOf(bool())) + controllerMethodMustReturnRule: anyOf(bool(), arrayOf(bool())) ]) conditionalTags: @@ -25,6 +27,8 @@ conditionalTags: phpstan.parser.richParserNodeVisitor: %cakeDC.addAssociationExistsTableClassRule% CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule: phpstan.rules.rule: %cakeDC.loadComponentExistsClassRule% + CakeDC\PHPStan\Rule\Controller\ControllerMethodMustReturnRule: + phpstan.rules.rule: %cakeDC.controllerMethodMustReturnRule% CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule: phpstan.rules.rule: %cakeDC.addAssociationExistsTableClassRule% CakeDC\PHPStan\Rule\Model\AddAssociationMatchOptionsTypesRule: @@ -45,6 +49,8 @@ services: class: CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor - class: CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule + - + class: CakeDC\PHPStan\Rule\Controller\ControllerMethodMustReturnRule - class: CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule - diff --git a/src/Method/AssociationTableMixinClassReflectionExtension.php b/src/Method/AssociationTableMixinClassReflectionExtension.php index 52e578f..4fed680 100644 --- a/src/Method/AssociationTableMixinClassReflectionExtension.php +++ b/src/Method/AssociationTableMixinClassReflectionExtension.php @@ -49,11 +49,11 @@ protected function getTableReflection(): ClassReflection public function hasMethod(ClassReflection $classReflection, string $methodName): bool { // magic findBy* method - if ($classReflection->isSubclassOf(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) { + if ($classReflection->is(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) { return true; } - if (!$classReflection->isSubclassOf(Association::class)) { + if (!$classReflection->is(Association::class)) { return false; } @@ -68,7 +68,7 @@ public function hasMethod(ClassReflection $classReflection, string $methodName): public function getMethod(ClassReflection $classReflection, string $methodName): MethodReflection { // magic findBy* method - if ($classReflection->isSubclassOf(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) { + if ($classReflection->is(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) { return new TableFindByPropertyMethodReflection($methodName, $classReflection); } @@ -82,11 +82,11 @@ public function getMethod(ClassReflection $classReflection, string $methodName): */ public function hasProperty(ClassReflection $classReflection, string $propertyName): bool { - if (!$classReflection->isSubclassOf(Association::class)) { + if (!$classReflection->is(Association::class)) { return false; } - return $this->getTableReflection()->hasProperty($propertyName); + return $this->getTableReflection()->hasInstanceProperty($propertyName); } /** diff --git a/src/Method/DummyParameter.php b/src/Method/DummyParameter.php index 443e3a5..a2c383a 100644 --- a/src/Method/DummyParameter.php +++ b/src/Method/DummyParameter.php @@ -48,7 +48,7 @@ public function __construct( bool $optional, ?PassedByReference $passedByReference, bool $variadic, - ?Type $defaultValue + ?Type $defaultValue, ) { $this->name = $name; $this->type = $type; diff --git a/src/Method/TableFindByPropertyMethodReflection.php b/src/Method/TableFindByPropertyMethodReflection.php index 7b2813c..a9b8b6b 100644 --- a/src/Method/TableFindByPropertyMethodReflection.php +++ b/src/Method/TableFindByPropertyMethodReflection.php @@ -45,13 +45,13 @@ public function __construct(string $name, ClassReflection $declaringClass) $this->name = $name; $this->declaringClass = $declaringClass; - $params = array_map(fn ($field) => new DummyParameter( + $params = array_map(fn($field) => new DummyParameter( $field, new MixedType(), false, null, false, - null + null, ), $this->getParams($name)); $returnType = new ObjectType(SelectQuery::class); @@ -62,7 +62,7 @@ public function __construct(string $name, ClassReflection $declaringClass) null, $params, false, - $returnType + $returnType, ), ]; } diff --git a/src/PhpDoc/TableAssociationTypeNodeResolverExtension.php b/src/PhpDoc/TableAssociationTypeNodeResolverExtension.php index 6fb3265..0013d58 100644 --- a/src/PhpDoc/TableAssociationTypeNodeResolverExtension.php +++ b/src/PhpDoc/TableAssociationTypeNodeResolverExtension.php @@ -80,7 +80,7 @@ public function resolve(TypeNode $typeNode, NameScope $nameScope): ?Type if ($config['table'] !== null && $config['association'] !== null) { return new GenericObjectType( $config['association']->getObjectClassNames()[0], - [$config['table']] + [$config['table']], ); } diff --git a/src/Rule/Controller/ControllerMethodMustReturnRule.php b/src/Rule/Controller/ControllerMethodMustReturnRule.php new file mode 100644 index 0000000..669169f --- /dev/null +++ b/src/Rule/Controller/ControllerMethodMustReturnRule.php @@ -0,0 +1,106 @@ + + */ + protected array $methodsRequiringReturn = [ + 'render', + 'redirect', + ]; + + /** + * @inheritDoc + */ + public function getNodeType(): string + { + return Expression::class; + } + + /** + * @param \PhpParser\Node $node + * @param \PHPStan\Analyser\Scope $scope + * @return list<\PHPStan\Rules\IdentifierRuleError> + */ + public function processNode(Node $node, Scope $scope): array + { + assert($node instanceof Expression); + + // Check if this is a method call + if (!$node->expr instanceof MethodCall) { + return []; + } + + $methodCall = $node->expr; + + // Check if the method name is one we care about + if (!$methodCall->name instanceof Node\Identifier) { + return []; + } + + $methodName = $methodCall->name->toString(); + if (!in_array($methodName, $this->methodsRequiringReturn, true)) { + return []; + } + + // Check if the method is being called on $this + $callerType = $scope->getType($methodCall->var); + if (!$callerType->isObject()->yes()) { + return []; + } + + // Check if it's a Controller class + $classReflections = $callerType->getObjectClassReflections(); + $isController = false; + foreach ($classReflections as $classReflection) { + if ($classReflection->is(Controller::class)) { + $isController = true; + break; + } + } + + if (!$isController) { + return []; + } + + // If we reach here, it means the method call is wrapped in an Expression node + // which means it's used as a statement (not returned) + return [ + RuleErrorBuilder::message(sprintf( + 'Method %s() must be returned to prevent unreachable code. Use "return $this->%s()" instead.', + $methodName, + $methodName, + )) + ->identifier('cake.controller.' . $methodName . 'MustReturn') + ->build(), + ]; + } +} diff --git a/src/Rule/LoadObjectExistsCakeClassRule.php b/src/Rule/LoadObjectExistsCakeClassRule.php index 7cbea77..6f4646b 100644 --- a/src/Rule/LoadObjectExistsCakeClassRule.php +++ b/src/Rule/LoadObjectExistsCakeClassRule.php @@ -71,7 +71,7 @@ public function processNode(Node $node, Scope $scope): array if ($inputClassName === null) { $inputClassName = $this->getInputClassName( $details['alias']->value, - $details['options'] + $details['options'], ); } if ($inputClassName === null || $this->getTargetClassName($inputClassName) !== null) { diff --git a/src/Rule/Model/AddAssociationMatchOptionsTypesRule.php b/src/Rule/Model/AddAssociationMatchOptionsTypesRule.php index 9dccf4d..c3f801c 100644 --- a/src/Rule/Model/AddAssociationMatchOptionsTypesRule.php +++ b/src/Rule/Model/AddAssociationMatchOptionsTypesRule.php @@ -96,7 +96,7 @@ public function processNode(Node $node, Scope $scope): array $details, $properties[$item->key->value], $item, - $scope + $scope, ); if ($error !== null) { $errors[] = $error; @@ -106,7 +106,7 @@ public function processNode(Node $node, Scope $scope): array 'Call to %s::%s with unknown option "%s".', $reference, $node->name->name, - $item->key->value + $item->key->value, )) ->identifier('cake.addAssociationWithValidOption.unknownOption') ->build(); @@ -164,13 +164,13 @@ protected function processPropertyTypeCheck( array $details, string $property, ArrayItem $item, - Scope $scope + Scope $scope, ): ?IdentifierRuleError { $object = new ObjectType($details['type']); $classReflection = $object->getClassReflection(); assert($classReflection instanceof ClassReflection); $propertyType = $classReflection - ->getProperty('_' . $property, $scope) + ->getInstanceProperty('_' . $property, $scope) ->getWritableType(); $assignedValueType = $scope->getType($item->value); $accepts = $this->ruleLevelHelper->accepts($propertyType, $assignedValueType, true); @@ -182,7 +182,7 @@ protected function processPropertyTypeCheck( 'Call to %s::%s with option "%s"', $details['reference'], $details['methodName'], - $item->key->value + $item->key->value, ); $verbosityLevel = VerbosityLevel::getRecommendedLevelByType($propertyType, $assignedValueType); @@ -191,8 +191,8 @@ protected function processPropertyTypeCheck( '%s (%s) does not accept %s.', $propertyDescription, $propertyType->describe($verbosityLevel), - $assignedValueType->describe($verbosityLevel) - ) + $assignedValueType->describe($verbosityLevel), + ), ) ->acceptsReasonsTip($accepts->reasons) ->identifier('cake.addAssociationWithValidOption.invalidType') diff --git a/src/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRule.php b/src/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRule.php index 40cbc34..a37db68 100644 --- a/src/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRule.php +++ b/src/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRule.php @@ -129,7 +129,7 @@ public function processNode(Node $node, Scope $scope): array $parameterType, $scope->getType($item), $details, - $name + $name, ) : null; if ($error !== null) { @@ -191,7 +191,7 @@ protected function processPropertyTypeCheck( Type $inputType, Type $assignedValueType, array $details, - string $property + string $property, ): ?RuleError { $accepts = $this->ruleLevelHelper->accepts($inputType, $assignedValueType, true); @@ -202,7 +202,7 @@ protected function processPropertyTypeCheck( 'Call to %s::%s with option "%s"', $details['reference'], $details['methodName'], - $property + $property, ); $verbosityLevel = VerbosityLevel::getRecommendedLevelByType($inputType, $assignedValueType); @@ -211,8 +211,8 @@ protected function processPropertyTypeCheck( '%s (%s) does not accept %s.', $propertyDescription, $inputType->describe($verbosityLevel), - $assignedValueType->describe($verbosityLevel) - ) + $assignedValueType->describe($verbosityLevel), + ), ) ->acceptsReasonsTip($accepts->reasons) ->identifier('cake.tableGetMatchOptionsTypes.invalidType') @@ -423,7 +423,7 @@ protected function checkMissingRequiredOptions(array $specificFinderOptions, arr 'Call to %s::%s is missing required finder option "%s".', $details['reference'], $details['methodName'], - $requireOptionName + $requireOptionName, ); $errors[] = RuleErrorBuilder::message($errorMessage) ->identifier('cake.tableGetMatchOptionsTypes.invalidType') diff --git a/src/Traits/BaseCakeRegistryReturnTrait.php b/src/Traits/BaseCakeRegistryReturnTrait.php index 8df3ab5..540ff6d 100644 --- a/src/Traits/BaseCakeRegistryReturnTrait.php +++ b/src/Traits/BaseCakeRegistryReturnTrait.php @@ -34,7 +34,7 @@ trait BaseCakeRegistryReturnTrait public function getTypeFromMethodCall( MethodReflection $methodReflection, MethodCall $methodCall, - Scope $scope + Scope $scope, ): ?Type { if (count($methodCall->getArgs()) === 0) { return null; diff --git a/src/Type/BaseTraitExpressionTypeResolverExtension.php b/src/Type/BaseTraitExpressionTypeResolverExtension.php index 0d88c5c..3b5d090 100644 --- a/src/Type/BaseTraitExpressionTypeResolverExtension.php +++ b/src/Type/BaseTraitExpressionTypeResolverExtension.php @@ -41,7 +41,7 @@ public function __construct( protected string $targetTrait, protected string $methodName, protected string $namespaceFormat, - protected ?string $propertyDefaultValue = null + protected ?string $propertyDefaultValue = null, ) { } diff --git a/src/Type/ControllerFetchTableDynamicReturnTypeExtension.php b/src/Type/ControllerFetchTableDynamicReturnTypeExtension.php index 6102969..9262551 100644 --- a/src/Type/ControllerFetchTableDynamicReturnTypeExtension.php +++ b/src/Type/ControllerFetchTableDynamicReturnTypeExtension.php @@ -27,7 +27,7 @@ class ControllerFetchTableDynamicReturnTypeExtension extends TableLocatorDynamic protected function getReturnTypeWithoutArgs( MethodReflection $methodReflection, MethodCall $methodCall, - ClassReflection $targetClassReflection + ClassReflection $targetClassReflection, ): ?Type { $type = parent::getReturnTypeWithoutArgs($methodReflection, $methodCall, $targetClassReflection); if ($type !== null) { @@ -47,7 +47,7 @@ protected function getReturnTypeWithoutArgs( */ protected function getDefaultTableByControllerClass(ClassReflection $targetClassReflection): ?string { - $hasProperty = $targetClassReflection->hasProperty('defaultTable'); + $hasProperty = $targetClassReflection->hasInstanceProperty('defaultTable'); if (!$hasProperty) { return null; } @@ -63,7 +63,7 @@ protected function getDefaultTableByControllerClass(ClassReflection $targetClass $tableClassName = sprintf( '%s\\Model\\Table\\%sTable', $baseNamespace, - $shortName + $shortName, ); if (class_exists($tableClassName)) { diff --git a/src/Type/RepositoryEntityDynamicReturnTypeExtension.php b/src/Type/RepositoryEntityDynamicReturnTypeExtension.php index 1745cf5..1b1fc5b 100644 --- a/src/Type/RepositoryEntityDynamicReturnTypeExtension.php +++ b/src/Type/RepositoryEntityDynamicReturnTypeExtension.php @@ -85,7 +85,7 @@ public function isMethodSupported(MethodReflection $methodReflection): bool public function getTypeFromMethodCall( MethodReflection $methodReflection, MethodCall $methodCall, - Scope $scope + Scope $scope, ): ?Type { $className = $this->getReferenceClass($scope, $methodCall); if ($className === null || $className === Table::class) { diff --git a/src/Type/RepositoryFirstArgIsTheReturnTypeExtension.php b/src/Type/RepositoryFirstArgIsTheReturnTypeExtension.php index fc60958..818868f 100644 --- a/src/Type/RepositoryFirstArgIsTheReturnTypeExtension.php +++ b/src/Type/RepositoryFirstArgIsTheReturnTypeExtension.php @@ -85,7 +85,7 @@ public function isMethodSupported(MethodReflection $methodReflection): bool public function getTypeFromMethodCall( MethodReflection $methodReflection, MethodCall $methodCall, - Scope $scope + Scope $scope, ): ?Type { $args = $methodCall->getArgs(); if (count($args) === 0) { diff --git a/src/Type/TableLocatorDynamicReturnTypeExtension.php b/src/Type/TableLocatorDynamicReturnTypeExtension.php index 954f844..2584872 100644 --- a/src/Type/TableLocatorDynamicReturnTypeExtension.php +++ b/src/Type/TableLocatorDynamicReturnTypeExtension.php @@ -65,7 +65,7 @@ public function __construct(string $className, string $methodName) public function getTypeFromMethodCall( MethodReflection $methodReflection, MethodCall $methodCall, - Scope $scope + Scope $scope, ): ?Type { if (count($methodCall->getArgs()) === 0) { $targetClassReflection = $this->getTargetClassReflection($scope, $methodCall); @@ -109,7 +109,7 @@ protected function getDefaultTable(ClassReflection $target): ?string protected function getReturnTypeWithoutArgs( MethodReflection $methodReflection, MethodCall $methodCall, - ClassReflection $targetClassReflection + ClassReflection $targetClassReflection, ): ?Type { try { $defaultTable = $this->getDefaultTable($targetClassReflection); diff --git a/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php b/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php new file mode 100644 index 0000000..6b0040d --- /dev/null +++ b/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php @@ -0,0 +1,62 @@ +analyse([__DIR__ . '/../../../test_app/Controller/FailingControllerMethodReturnLogic.php'], [ + [ + 'Method render() must be returned to prevent unreachable code. Use "return $this->render()" instead.', + 17, + ], + [ + 'Method redirect() must be returned to prevent unreachable code. Use "return $this->redirect()" instead.', + 29, + ], + [ + 'Method render() must be returned to prevent unreachable code. Use "return $this->render()" instead.', + 62, + ], + [ + 'Method redirect() must be returned to prevent unreachable code. Use "return $this->redirect()" instead.', + 74, + ], + ]); + } + + /** + * @inheritDoc + */ + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/../../../../extension.neon']; + } +} diff --git a/tests/TestCase/Rule/Model/AddAssociationMatchOptionsTypesRuleTest.php b/tests/TestCase/Rule/Model/AddAssociationMatchOptionsTypesRuleTest.php index 96ce888..336d352 100644 --- a/tests/TestCase/Rule/Model/AddAssociationMatchOptionsTypesRuleTest.php +++ b/tests/TestCase/Rule/Model/AddAssociationMatchOptionsTypesRuleTest.php @@ -17,7 +17,7 @@ protected function getRule(): Rule { // getRule() method needs to return an instance of the tested rule return new AddAssociationMatchOptionsTypesRule( - static::getContainer()->getByType(RuleLevelHelper::class) + static::getContainer()->getByType(RuleLevelHelper::class), ); } diff --git a/tests/TestCase/Rule/Model/Fake/FailingOrmFindRuleItemsLogic.php b/tests/TestCase/Rule/Model/Fake/FailingOrmFindRuleItemsLogic.php index b8b9a01..79ff7ce 100644 --- a/tests/TestCase/Rule/Model/Fake/FailingOrmFindRuleItemsLogic.php +++ b/tests/TestCase/Rule/Model/Fake/FailingOrmFindRuleItemsLogic.php @@ -49,7 +49,7 @@ public function process() groupBy: false, having: new stdClass(), contain: true, - page: 'Other' + page: 'Other', ); $Table->find( 'all', //Good options @@ -60,7 +60,7 @@ public function process() offset: 3, group: ['Notes.type'], contain: ['Users'], - page: 3 + page: 3, ); $Table->find( 'all', //some good options but not all @@ -71,7 +71,7 @@ public function process() offset: 3, group: true, //bad contain: ['Users'], - page: 3 + page: 3, ); $query = $Table->find(); $query->find(//bad information @@ -123,13 +123,13 @@ public function process() 'featured', //custom finder is known fields: ['Notes.id', 'Notes.note', 'Notes.created'], year: 2024, - fun: true + fun: true, ); $Table->find( 'featured', //custom finder is known but options are invalid fields: ['Notes.id', 'Notes.note', 'Notes.created'], year: 10.0, - fun: 1 + fun: 1, ); $Table->find( 'featured', //custom finder is known but required options are missing only have basic find options @@ -165,18 +165,18 @@ public function process() $Table->find( 'optionsPacked', sort: ['Notes.note' => 'ASC'], - labelField: 'id' + labelField: 'id', ); $Table->find( 'optionsPacked', sort: ['Notes.note' => 'ASC'], - labelField: 'id' + labelField: 'id', ); $Table->find('argsPacked'); $Table->find( 'argsPacked', sort: ['Notes.note' => 'ASC'], - groupLabel: 'type' + groupLabel: 'type', ); $Table->find('argsPacked', [ 'sort' => ['Notes.note' => 'ASC'], @@ -192,13 +192,13 @@ public function process() $Table->find( 'twoArgsButNotLegacy', sort: ['Notes.note' => 'ASC'], - myType: 'featured' + myType: 'featured', ); $Table->find('twoArgsButNotLegacy'); $Table->find( 'twoArgsButNotLegacy', sort: ['Notes.note' => 'ASC'], - myType: 19 + myType: 19, ); $field = $Table->getTypeTestTwoArgsButNotLegacy(); $value = 'featured'; diff --git a/tests/TestCase/Rule/Model/Fake/FailingRuleItemsTable.php b/tests/TestCase/Rule/Model/Fake/FailingRuleItemsTable.php index 8172a5e..24a2fde 100644 --- a/tests/TestCase/Rule/Model/Fake/FailingRuleItemsTable.php +++ b/tests/TestCase/Rule/Model/Fake/FailingRuleItemsTable.php @@ -68,7 +68,7 @@ public function initialize(array $config): void 'cascadeCallbacks' => 1,//Can't be integer, it should be bool 'conditions' => 'Users.active = 1',//Can't be string, it should be Closure or array 'dependent' => 0,//Must be - 'finder' => fn () => 'f', + 'finder' => fn() => 'f', 'bindingKey' => 10, 'foreignKey' => 11, 'joinType' => 12, @@ -97,11 +97,11 @@ public function initialize(array $config): void ]); $this->belongsToMany('SadUsers', [ 'className' => UsersTable::class, - 'targetForeignKey' => fn () => 10, + 'targetForeignKey' => fn() => 10, 'through' => new stdClass(), - 'saveStrategy' => fn () => 'na', + 'saveStrategy' => fn() => 'na', 'sort' => false, - 'joinTable' => fn () => 'my_users_failing', + 'joinTable' => fn() => 'my_users_failing', ]); $this->hasOne('MainArticles', [ 'className' => VeryCustomize00009ArticlesTable::class, @@ -122,7 +122,7 @@ public function initialize(array $config): void 'cascadeCallbacks' => 1,//Can't be integer, it should be bool 'conditions' => 'parent_id = id',//Can't be string, it should be Closure or array 'dependent' => 0,//Must be - 'finder' => fn () => 'f', + 'finder' => fn() => 'f', 'bindingKey' => 10, 'foreignKey' => 11, 'joinType' => 12, @@ -150,7 +150,7 @@ public function initialize(array $config): void 'cascadeCallbacks' => 1,//Can't be integer, it should be bool 'conditions' => 'parent_id = id',//Can't be string, it should be Closure or array 'dependent' => 0,//Must be - 'finder' => fn () => 'f', + 'finder' => fn() => 'f', 'bindingKey' => 10, 'foreignKey' => 11, 'joinType' => 12, diff --git a/tests/TestCase/Rule/Model/Fake/FailingTableGetRuleItemsLogic.php b/tests/TestCase/Rule/Model/Fake/FailingTableGetRuleItemsLogic.php index 5e98901..c4a4685 100644 --- a/tests/TestCase/Rule/Model/Fake/FailingTableGetRuleItemsLogic.php +++ b/tests/TestCase/Rule/Model/Fake/FailingTableGetRuleItemsLogic.php @@ -49,7 +49,7 @@ public function process() groupBy: false, having: new stdClass(), contain: true, - page: 'Other' + page: 'Other', ); $Table->get( 1, //Good options @@ -60,7 +60,7 @@ public function process() offset: 3, group: ['Notes.type'], contain: ['Users'], - page: 3 + page: 3, ); $Table->get(1, 'all', ...[ 'order' => false, diff --git a/tests/TestCase/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRuleTest.php b/tests/TestCase/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRuleTest.php index 273886c..8331638 100644 --- a/tests/TestCase/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRuleTest.php +++ b/tests/TestCase/Rule/Model/OrmSelectQueryFindMatchOptionsTypesRuleTest.php @@ -17,7 +17,7 @@ protected function getRule(): Rule { // getRule() method needs to return an instance of the tested rule return new OrmSelectQueryFindMatchOptionsTypesRule( - static::getContainer()->getByType(RuleLevelHelper::class) + static::getContainer()->getByType(RuleLevelHelper::class), ); } diff --git a/tests/TestCase/Rule/Model/TableGetMatchOptionsTypesRuleTest.php b/tests/TestCase/Rule/Model/TableGetMatchOptionsTypesRuleTest.php index 16cc824..fba9259 100644 --- a/tests/TestCase/Rule/Model/TableGetMatchOptionsTypesRuleTest.php +++ b/tests/TestCase/Rule/Model/TableGetMatchOptionsTypesRuleTest.php @@ -17,7 +17,7 @@ protected function getRule(): Rule { // getRule() method needs to return an instance of the tested rule return new TableGetMatchOptionsTypesRule( - static::getContainer()->getByType(RuleLevelHelper::class) + static::getContainer()->getByType(RuleLevelHelper::class), ); } diff --git a/tests/test_app/Controller/FailingControllerMethodReturnLogic.php b/tests/test_app/Controller/FailingControllerMethodReturnLogic.php new file mode 100644 index 0000000..075f27a --- /dev/null +++ b/tests/test_app/Controller/FailingControllerMethodReturnLogic.php @@ -0,0 +1,77 @@ +render('edit'); + // This code is unreachable + $this->set('data', 'value'); + } + + /** + * Action with redirect() not returned + * + * @return \Cake\Http\Response|null + */ + public function actionWithoutReturnRedirect() + { + $this->redirect(['action' => 'index']); + // This code is unreachable + $this->set('data', 'value'); + } + + /** + * Action with render() properly returned + * + * @return \Cake\Http\Response|null + */ + public function actionWithCorrectRender() + { + return $this->render('edit'); + } + + /** + * Action with redirect() properly returned + * + * @return \Cake\Http\Response|null + */ + public function actionWithCorrectRedirect() + { + return $this->redirect(['action' => 'index']); + } + + /** + * Action with conditional render() not returned + * + * @return \Cake\Http\Response|null + */ + public function actionWithConditionalRenderNoReturn() + { + if ($this->request->is('ajax')) { + $this->render('ajax_view'); + } + } + + /** + * Action with conditional redirect() not returned + * + * @return \Cake\Http\Response|null + */ + public function actionWithConditionalRedirectNoReturn() + { + if (!$this->request->getData('id')) { + $this->redirect(['action' => 'add']); + } + } +} diff --git a/tests/test_app/Model/Table/NotesTable.php b/tests/test_app/Model/Table/NotesTable.php index 9ce9669..d6e33ec 100644 --- a/tests/test_app/Model/Table/NotesTable.php +++ b/tests/test_app/Model/Table/NotesTable.php @@ -65,7 +65,7 @@ public function warning(): array $this->find( 'twoArgsButNotLegacy', sort: ['Notes.note' => 'ASC'], - myType: 'featured' + myType: 'featured', ); $this->find('argsPacked'); @@ -89,7 +89,7 @@ public function findFeatured(SelectQuery $query, string|int $year, bool $fun): S if ($fun === true) { $where[] = $query->newExpr()->in( 'type', - ['funny_stuff', 'funny_songs', 'funny_messages'] + ['funny_stuff', 'funny_songs', 'funny_messages'], ); } diff --git a/tests/test_app/Model/Table/VeryCustomize00009ArticlesTable.php b/tests/test_app/Model/Table/VeryCustomize00009ArticlesTable.php index 3fc87e8..eb6c90f 100644 --- a/tests/test_app/Model/Table/VeryCustomize00009ArticlesTable.php +++ b/tests/test_app/Model/Table/VeryCustomize00009ArticlesTable.php @@ -66,7 +66,7 @@ public function newSample() [ 'title' => 'This is my title', 'content' => 'Sample content for test', - ] + ], ); } } From 7e33f0782a071b6a6ee52483a5704699e41307b2 Mon Sep 17 00:00:00 2001 From: mscherer Date: Sun, 12 Oct 2025 12:06:47 +0200 Subject: [PATCH 2/6] Adjust constraint. --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 4bcf8b0..b154ab0 100644 --- a/composer.json +++ b/composer.json @@ -12,7 +12,7 @@ ], "require": { "php": ">=8.1.0", - "phpstan/phpstan": "^2.0", + "phpstan/phpstan": "^2.1", "cakephp/cakephp": "^5.0" }, "require-dev": { From 7f5a0f0d14ef1d3af505d9879642e916f2d0f85a Mon Sep 17 00:00:00 2001 From: mscherer Date: Sun, 12 Oct 2025 12:09:01 +0200 Subject: [PATCH 3/6] Use PHPUnit 12. --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index b154ab0..72e8dfe 100644 --- a/composer.json +++ b/composer.json @@ -12,12 +12,12 @@ ], "require": { "php": ">=8.1.0", - "phpstan/phpstan": "^2.1", + "phpstan/phpstan": "^2.1.26", "cakephp/cakephp": "^5.0" }, "require-dev": { "phpstan/phpstan-phpunit": "^2.0", - "phpunit/phpunit": "^10.1", + "phpunit/phpunit": "^12.1", "cakephp/cakephp-codesniffer": "^5.0", "phpstan/phpstan-deprecation-rules": "^2.0", "phpstan/phpstan-strict-rules": "^2.0" From 748aa012d4327f388385d21ada80048d793a3f97 Mon Sep 17 00:00:00 2001 From: mscherer Date: Sun, 12 Oct 2025 12:10:28 +0200 Subject: [PATCH 4/6] Use PHPUnit 12. --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 72e8dfe..8edb7a2 100644 --- a/composer.json +++ b/composer.json @@ -17,7 +17,7 @@ }, "require-dev": { "phpstan/phpstan-phpunit": "^2.0", - "phpunit/phpunit": "^12.1", + "phpunit/phpunit": "^10.5 || ^11.5 || ^12.1", "cakephp/cakephp-codesniffer": "^5.0", "phpstan/phpstan-deprecation-rules": "^2.0", "phpstan/phpstan-strict-rules": "^2.0" From 95154b897a8e34f2381c47968c672d4bbd8c59ee Mon Sep 17 00:00:00 2001 From: mscherer Date: Sun, 12 Oct 2025 12:18:49 +0200 Subject: [PATCH 5/6] Fix up testing files. --- .../ControllerMethodMustReturnRuleTest.php | 2 +- .../FailingControllerMethodReturnLogic.php | 2 +- tests/data/Rule/addBehaviorRule.php | 43 ------------------- 3 files changed, 2 insertions(+), 45 deletions(-) rename tests/{test_app/Controller => TestCase/Rule/Controller/Fake}/FailingControllerMethodReturnLogic.php (96%) delete mode 100644 tests/data/Rule/addBehaviorRule.php diff --git a/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php b/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php index 6b0040d..e2394d3 100644 --- a/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php +++ b/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php @@ -32,7 +32,7 @@ protected function getRule(): Rule */ public function testRule(): void { - $this->analyse([__DIR__ . '/../../../test_app/Controller/FailingControllerMethodReturnLogic.php'], [ + $this->analyse([__DIR__ . '/Fake/FailingControllerMethodReturnLogic.php'], [ [ 'Method render() must be returned to prevent unreachable code. Use "return $this->render()" instead.', 17, diff --git a/tests/test_app/Controller/FailingControllerMethodReturnLogic.php b/tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php similarity index 96% rename from tests/test_app/Controller/FailingControllerMethodReturnLogic.php rename to tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php index 075f27a..811bb83 100644 --- a/tests/test_app/Controller/FailingControllerMethodReturnLogic.php +++ b/tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php @@ -1,7 +1,7 @@ setTable('notes'); - $this->setDisplayField('note'); - $this->setPrimaryKey('id'); - $this->addBehavior('Timestamp');//valid name - $this->addBehavior('Timtamp');//Invalid name - $this->addBehavior('Tree', [ - 'className' => 'MyTreeBehavior',//invalid name - ]); - $this->addBehavior('Translate', [ - 'className' => 'Cake\Behavior\TranslateBehavior',//invalid full className - ]); - $this->addBehavior('CounterCache', [ - 'className' => 'Cake\ORM\Behavior\CounterCacheBehavior',//valid className - ]); - $this->addBehavior('SampleTestCustomMethod');//valid behavior from test_app - } -} From dd54ceae236f4acda8954afecbf452ffc0291c6b Mon Sep 17 00:00:00 2001 From: mscherer Date: Sun, 12 Oct 2025 12:54:19 +0200 Subject: [PATCH 6/6] Adjust name to reflect also assignment as OK. --- README.md | 12 ++++++++-- rules.neon | 10 ++++---- ...php => ControllerMethodMustBeUsedRule.php} | 19 ++++++++------- ...=> ControllerMethodMustBeUsedRuleTest.php} | 14 +++++------ .../FailingControllerMethodReturnLogic.php | 24 +++++++++++++++++++ 5 files changed, 56 insertions(+), 23 deletions(-) rename src/Rule/Controller/{ControllerMethodMustReturnRule.php => ControllerMethodMustBeUsedRule.php} (80%) rename tests/TestCase/Rule/Controller/{ControllerMethodMustReturnRuleTest.php => ControllerMethodMustBeUsedRuleTest.php} (61%) diff --git a/README.md b/README.md index 02e1cf4..7404ced 100644 --- a/README.md +++ b/README.md @@ -148,8 +148,8 @@ This rule check if the options (args) passed to Table::find and SelectQuery are ### TableGetMatchOptionsTypesRule This rule check if the options (args) passed to Table::get are valid find options types. -### ControllerMethodMustReturnRule -This rule enforces that controller methods like `render()` and `redirect()` must be returned to prevent unreachable code. These methods should always be used with a `return` statement to make the control flow explicit. +### ControllerMethodMustBeUsedRule +This rule enforces that controller methods like `render()` and `redirect()` must be used (returned or assigned) to prevent unreachable code. These methods should not be called in void context - use them with a `return` statement or assign them to a variable to make the control flow explicit.
Examples: @@ -168,6 +168,14 @@ public function myAction() return $this->render('edit'); } +// Also good - assignment is valid +public function myAction() +{ + $response = $this->render('edit'); + + return $response; +} + // Bad - code after redirect() is unreachable public function myAction() { diff --git a/rules.neon b/rules.neon index 901db64..4b637f4 100644 --- a/rules.neon +++ b/rules.neon @@ -8,7 +8,7 @@ parameters: disallowEntityArrayAccessRule: false getMailerExistsClassRule: true loadComponentExistsClassRule: true - controllerMethodMustReturnRule: true + controllerMethodMustBeUsedRule: true parametersSchema: cakeDC: structure([ addAssociationExistsTableClassRule: anyOf(bool(), arrayOf(bool())) @@ -19,7 +19,7 @@ parametersSchema: disallowEntityArrayAccessRule: anyOf(bool(), arrayOf(bool())) getMailerExistsClassRule: anyOf(bool(), arrayOf(bool())) loadComponentExistsClassRule: anyOf(bool(), arrayOf(bool())) - controllerMethodMustReturnRule: anyOf(bool(), arrayOf(bool())) + controllerMethodMustBeUsedRule: anyOf(bool(), arrayOf(bool())) ]) conditionalTags: @@ -27,8 +27,8 @@ conditionalTags: phpstan.parser.richParserNodeVisitor: %cakeDC.addAssociationExistsTableClassRule% CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule: phpstan.rules.rule: %cakeDC.loadComponentExistsClassRule% - CakeDC\PHPStan\Rule\Controller\ControllerMethodMustReturnRule: - phpstan.rules.rule: %cakeDC.controllerMethodMustReturnRule% + CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule: + phpstan.rules.rule: %cakeDC.controllerMethodMustBeUsedRule% CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule: phpstan.rules.rule: %cakeDC.addAssociationExistsTableClassRule% CakeDC\PHPStan\Rule\Model\AddAssociationMatchOptionsTypesRule: @@ -50,7 +50,7 @@ services: - class: CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule - - class: CakeDC\PHPStan\Rule\Controller\ControllerMethodMustReturnRule + class: CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule - class: CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule - diff --git a/src/Rule/Controller/ControllerMethodMustReturnRule.php b/src/Rule/Controller/ControllerMethodMustBeUsedRule.php similarity index 80% rename from src/Rule/Controller/ControllerMethodMustReturnRule.php rename to src/Rule/Controller/ControllerMethodMustBeUsedRule.php index 669169f..680e128 100644 --- a/src/Rule/Controller/ControllerMethodMustReturnRule.php +++ b/src/Rule/Controller/ControllerMethodMustBeUsedRule.php @@ -22,17 +22,17 @@ use PHPStan\Rules\RuleErrorBuilder; /** - * Rule to enforce that certain controller methods must be returned - * to prevent unreachable code. + * Rule to enforce that certain controller methods must be used + * (returned or assigned) to prevent unreachable code. */ -class ControllerMethodMustReturnRule implements Rule +class ControllerMethodMustBeUsedRule implements Rule { /** - * Methods that must be returned + * Methods that must be used (returned or assigned) * * @var array */ - protected array $methodsRequiringReturn = [ + protected array $methodsRequiringUsage = [ 'render', 'redirect', ]; @@ -67,7 +67,7 @@ public function processNode(Node $node, Scope $scope): array } $methodName = $methodCall->name->toString(); - if (!in_array($methodName, $this->methodsRequiringReturn, true)) { + if (!in_array($methodName, $this->methodsRequiringUsage, true)) { return []; } @@ -92,14 +92,15 @@ public function processNode(Node $node, Scope $scope): array } // If we reach here, it means the method call is wrapped in an Expression node - // which means it's used as a statement (not returned) + // which means it's used as a statement (not returned or assigned) return [ RuleErrorBuilder::message(sprintf( - 'Method %s() must be returned to prevent unreachable code. Use "return $this->%s()" instead.', + 'Method `%s()` must be used to prevent unreachable code. ' . + 'Use `return $this->%s()` or assign it to a variable.', $methodName, $methodName, )) - ->identifier('cake.controller.' . $methodName . 'MustReturn') + ->identifier('cake.controller.' . $methodName . 'MustBeUsed') ->build(), ]; } diff --git a/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php b/tests/TestCase/Rule/Controller/ControllerMethodMustBeUsedRuleTest.php similarity index 61% rename from tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php rename to tests/TestCase/Rule/Controller/ControllerMethodMustBeUsedRuleTest.php index e2394d3..4d4e6eb 100644 --- a/tests/TestCase/Rule/Controller/ControllerMethodMustReturnRuleTest.php +++ b/tests/TestCase/Rule/Controller/ControllerMethodMustBeUsedRuleTest.php @@ -13,18 +13,18 @@ namespace CakeDC\PHPStan\Test\TestCase\Rule\Controller; -use CakeDC\PHPStan\Rule\Controller\ControllerMethodMustReturnRule; +use CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; -class ControllerMethodMustReturnRuleTest extends RuleTestCase +class ControllerMethodMustBeUsedRuleTest extends RuleTestCase { /** * @return \PHPStan\Rules\Rule */ protected function getRule(): Rule { - return new ControllerMethodMustReturnRule(); + return new ControllerMethodMustBeUsedRule(); } /** @@ -34,19 +34,19 @@ public function testRule(): void { $this->analyse([__DIR__ . '/Fake/FailingControllerMethodReturnLogic.php'], [ [ - 'Method render() must be returned to prevent unreachable code. Use "return $this->render()" instead.', + 'Method `render()` must be used to prevent unreachable code. Use `return $this->render()` or assign it to a variable.', 17, ], [ - 'Method redirect() must be returned to prevent unreachable code. Use "return $this->redirect()" instead.', + 'Method `redirect()` must be used to prevent unreachable code. Use `return $this->redirect()` or assign it to a variable.', 29, ], [ - 'Method render() must be returned to prevent unreachable code. Use "return $this->render()" instead.', + 'Method `render()` must be used to prevent unreachable code. Use `return $this->render()` or assign it to a variable.', 62, ], [ - 'Method redirect() must be returned to prevent unreachable code. Use "return $this->redirect()" instead.', + 'Method `redirect()` must be used to prevent unreachable code. Use `return $this->redirect()` or assign it to a variable.', 74, ], ]); diff --git a/tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php b/tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php index 811bb83..7d5d0be 100644 --- a/tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php +++ b/tests/TestCase/Rule/Controller/Fake/FailingControllerMethodReturnLogic.php @@ -74,4 +74,28 @@ public function actionWithConditionalRedirectNoReturn() $this->redirect(['action' => 'add']); } } + + /** + * Action with render() assigned to variable (valid usage) + * + * @return \Cake\Http\Response|null + */ + public function actionWithRenderAssigned() + { + $response = $this->render('edit'); + + return $response; + } + + /** + * Action with redirect() assigned to variable (valid usage) + * + * @return \Cake\Http\Response|null + */ + public function actionWithRedirectAssigned() + { + $response = $this->redirect(['action' => 'index']); + + return $response; + } }