Skip to content

Commit 2c77907

Browse files
committed
Merge remote-tracking branch 'own/controller-returns' into bugfix-assoc-reflection
2 parents 4742e4c + dd54cea commit 2c77907

30 files changed

+387
-116
lines changed

.github/workflows/ci.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ jobs:
1919
strategy:
2020
fail-fast: false
2121
matrix:
22-
php-version: ['8.1', '8.2', '8.3']
22+
php-version: ['8.1', '8.4']
2323
dependencies: ['highest']
2424
include:
2525
- php-version: '8.1'
@@ -68,7 +68,7 @@ jobs:
6868
uses: ramsey/composer-install@v2
6969

7070
- name: Run phpcs
71-
run: vendor/bin/phpcs --report=checkstyle src/ tests/ | cs2pr
71+
run: vendor/bin/phpcs --report=checkstyle | cs2pr
7272

7373
- name: Run phpstan
7474
if: always()

README.md

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -148,13 +148,48 @@ This rule check if the options (args) passed to Table::find and SelectQuery are
148148
### TableGetMatchOptionsTypesRule
149149
This rule check if the options (args) passed to Table::get are valid find options types.
150150

151-
To enable this rule update your phpstan.neon with:
151+
### ControllerMethodMustBeUsedRule
152+
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.
152153

154+
<details>
155+
<summary>Examples:</summary>
156+
157+
```php
158+
// Bad - code after render() is unreachable
159+
public function myAction()
160+
{
161+
$this->render('edit');
162+
$this->set('data', 'value'); // This will never execute
163+
}
164+
165+
// Good - explicit return prevents confusion
166+
public function myAction()
167+
{
168+
return $this->render('edit');
169+
}
170+
171+
// Also good - assignment is valid
172+
public function myAction()
173+
{
174+
$response = $this->render('edit');
175+
176+
return $response;
177+
}
178+
179+
// Bad - code after redirect() is unreachable
180+
public function myAction()
181+
{
182+
$this->redirect(['action' => 'index']);
183+
$this->Flash->success('Done'); // This will never execute
184+
}
185+
186+
// Good - explicit return prevents confusion
187+
public function myAction()
188+
{
189+
return $this->redirect(['action' => 'index']);
190+
}
153191
```
154-
parameters:
155-
cakeDC:
156-
disallowEntityArrayAccessRule: true
157-
```
192+
</details>
158193

159194
### How to disable a rule
160195
Each rule has a parameter in cakeDC 'namespace' to enable or disable, it is the same name of the

composer.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@
1212
],
1313
"require": {
1414
"php": ">=8.1.0",
15-
"phpstan/phpstan": "^2.0",
15+
"phpstan/phpstan": "^2.1.26",
1616
"cakephp/cakephp": "^5.0"
1717
},
1818
"require-dev": {
1919
"phpstan/phpstan-phpunit": "^2.0",
20-
"phpunit/phpunit": "^10.1",
20+
"phpunit/phpunit": "^10.5 || ^11.5 || ^12.1",
2121
"cakephp/cakephp-codesniffer": "^5.0",
2222
"phpstan/phpstan-deprecation-rules": "^2.0",
2323
"phpstan/phpstan-strict-rules": "^2.0"
@@ -42,8 +42,8 @@
4242
}
4343
},
4444
"scripts": {
45-
"cs-check": "phpcs -p src/ tests",
46-
"cs-fix": "phpcbf -p src/ tests",
45+
"cs-check": "phpcs -p",
46+
"cs-fix": "phpcbf -p",
4747
"test": "phpunit --stderr",
4848
"stan-integration": [
4949
"phpstan analyse --debug -c phpstan-test-app.neon",

phpcs.xml

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
<?xml version="1.0"?>
2-
<ruleset name="CakePHP Core">
3-
<config name="installed_paths" value="../../cakephp/cakephp-codesniffer" />
2+
<ruleset name="cakephp-phpstan">
3+
<rule ref="CakePHP"/>
44

5-
<rule ref="CakePHP" />
5+
<arg value="s"/>
66

7-
<arg value="s"/>
8-
</ruleset>
7+
<file>src/</file>
8+
<file>tests/</file>
9+
10+
<exclude-pattern>/tests/data/</exclude-pattern>
11+
</ruleset>

rules.neon

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ parameters:
88
disallowEntityArrayAccessRule: false
99
getMailerExistsClassRule: true
1010
loadComponentExistsClassRule: true
11+
controllerMethodMustBeUsedRule: true
1112
parametersSchema:
1213
cakeDC: structure([
1314
addAssociationExistsTableClassRule: anyOf(bool(), arrayOf(bool()))
@@ -18,13 +19,16 @@ parametersSchema:
1819
disallowEntityArrayAccessRule: anyOf(bool(), arrayOf(bool()))
1920
getMailerExistsClassRule: anyOf(bool(), arrayOf(bool()))
2021
loadComponentExistsClassRule: anyOf(bool(), arrayOf(bool()))
22+
controllerMethodMustBeUsedRule: anyOf(bool(), arrayOf(bool()))
2123
])
2224

2325
conditionalTags:
2426
CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor:
2527
phpstan.parser.richParserNodeVisitor: %cakeDC.addAssociationExistsTableClassRule%
2628
CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule:
2729
phpstan.rules.rule: %cakeDC.loadComponentExistsClassRule%
30+
CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule:
31+
phpstan.rules.rule: %cakeDC.controllerMethodMustBeUsedRule%
2832
CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule:
2933
phpstan.rules.rule: %cakeDC.addAssociationExistsTableClassRule%
3034
CakeDC\PHPStan\Rule\Model\AddAssociationMatchOptionsTypesRule:
@@ -45,6 +49,8 @@ services:
4549
class: CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor
4650
-
4751
class: CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule
52+
-
53+
class: CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule
4854
-
4955
class: CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule
5056
-

src/Method/AssociationTableMixinClassReflectionExtension.php

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,11 @@ protected function getTableReflection(): ClassReflection
4949
public function hasMethod(ClassReflection $classReflection, string $methodName): bool
5050
{
5151
// magic findBy* method
52-
if ($classReflection->isSubclassOf(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
52+
if ($classReflection->is(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
5353
return true;
5454
}
5555

56-
if (!$classReflection->isSubclassOf(Association::class)) {
56+
if (!$classReflection->is(Association::class)) {
5757
return false;
5858
}
5959

@@ -73,7 +73,7 @@ public function hasMethod(ClassReflection $classReflection, string $methodName):
7373
public function getMethod(ClassReflection $classReflection, string $methodName): MethodReflection
7474
{
7575
// magic findBy* method
76-
if ($classReflection->isSubclassOf(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
76+
if ($classReflection->is(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
7777
return new TableFindByPropertyMethodReflection($methodName, $classReflection);
7878
}
7979

@@ -92,11 +92,11 @@ public function getMethod(ClassReflection $classReflection, string $methodName):
9292
*/
9393
public function hasProperty(ClassReflection $classReflection, string $propertyName): bool
9494
{
95-
if (!$classReflection->isSubclassOf(Association::class)) {
95+
if (!$classReflection->is(Association::class)) {
9696
return false;
9797
}
9898

99-
return $this->getTableReflection()->hasProperty($propertyName);
99+
return $this->getTableReflection()->hasInstanceProperty($propertyName);
100100
}
101101

102102
/**

src/Method/DummyParameter.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public function __construct(
4848
bool $optional,
4949
?PassedByReference $passedByReference,
5050
bool $variadic,
51-
?Type $defaultValue
51+
?Type $defaultValue,
5252
) {
5353
$this->name = $name;
5454
$this->type = $type;

src/Method/TableFindByPropertyMethodReflection.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,13 @@ public function __construct(string $name, ClassReflection $declaringClass)
4545
$this->name = $name;
4646

4747
$this->declaringClass = $declaringClass;
48-
$params = array_map(fn ($field) => new DummyParameter(
48+
$params = array_map(fn($field) => new DummyParameter(
4949
$field,
5050
new MixedType(),
5151
false,
5252
null,
5353
false,
54-
null
54+
null,
5555
), $this->getParams($name));
5656

5757
$returnType = new ObjectType(SelectQuery::class);
@@ -62,7 +62,7 @@ public function __construct(string $name, ClassReflection $declaringClass)
6262
null,
6363
$params,
6464
false,
65-
$returnType
65+
$returnType,
6666
),
6767
];
6868
}

src/PhpDoc/TableAssociationTypeNodeResolverExtension.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public function resolve(TypeNode $typeNode, NameScope $nameScope): ?Type
8080
if ($config['table'] !== null && $config['association'] !== null) {
8181
return new GenericObjectType(
8282
$config['association']->getObjectClassNames()[0],
83-
[$config['table']]
83+
[$config['table']],
8484
);
8585
}
8686

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
/**
5+
* Copyright 2024, Cake Development Corporation (https://www.cakedc.com)
6+
*
7+
* Licensed under The MIT License
8+
* Redistributions of files must retain the above copyright notice.
9+
*
10+
* @copyright Copyright 2024, Cake Development Corporation (https://www.cakedc.com)
11+
* @license MIT License (http://www.opensource.org/licenses/mit-license.php)
12+
*/
13+
14+
namespace CakeDC\PHPStan\Rule\Controller;
15+
16+
use Cake\Controller\Controller;
17+
use PhpParser\Node;
18+
use PhpParser\Node\Expr\MethodCall;
19+
use PhpParser\Node\Stmt\Expression;
20+
use PHPStan\Analyser\Scope;
21+
use PHPStan\Rules\Rule;
22+
use PHPStan\Rules\RuleErrorBuilder;
23+
24+
/**
25+
* Rule to enforce that certain controller methods must be used
26+
* (returned or assigned) to prevent unreachable code.
27+
*/
28+
class ControllerMethodMustBeUsedRule implements Rule
29+
{
30+
/**
31+
* Methods that must be used (returned or assigned)
32+
*
33+
* @var array<string>
34+
*/
35+
protected array $methodsRequiringUsage = [
36+
'render',
37+
'redirect',
38+
];
39+
40+
/**
41+
* @inheritDoc
42+
*/
43+
public function getNodeType(): string
44+
{
45+
return Expression::class;
46+
}
47+
48+
/**
49+
* @param \PhpParser\Node $node
50+
* @param \PHPStan\Analyser\Scope $scope
51+
* @return list<\PHPStan\Rules\IdentifierRuleError>
52+
*/
53+
public function processNode(Node $node, Scope $scope): array
54+
{
55+
assert($node instanceof Expression);
56+
57+
// Check if this is a method call
58+
if (!$node->expr instanceof MethodCall) {
59+
return [];
60+
}
61+
62+
$methodCall = $node->expr;
63+
64+
// Check if the method name is one we care about
65+
if (!$methodCall->name instanceof Node\Identifier) {
66+
return [];
67+
}
68+
69+
$methodName = $methodCall->name->toString();
70+
if (!in_array($methodName, $this->methodsRequiringUsage, true)) {
71+
return [];
72+
}
73+
74+
// Check if the method is being called on $this
75+
$callerType = $scope->getType($methodCall->var);
76+
if (!$callerType->isObject()->yes()) {
77+
return [];
78+
}
79+
80+
// Check if it's a Controller class
81+
$classReflections = $callerType->getObjectClassReflections();
82+
$isController = false;
83+
foreach ($classReflections as $classReflection) {
84+
if ($classReflection->is(Controller::class)) {
85+
$isController = true;
86+
break;
87+
}
88+
}
89+
90+
if (!$isController) {
91+
return [];
92+
}
93+
94+
// If we reach here, it means the method call is wrapped in an Expression node
95+
// which means it's used as a statement (not returned or assigned)
96+
return [
97+
RuleErrorBuilder::message(sprintf(
98+
'Method `%s()` must be used to prevent unreachable code. ' .
99+
'Use `return $this->%s()` or assign it to a variable.',
100+
$methodName,
101+
$methodName,
102+
))
103+
->identifier('cake.controller.' . $methodName . 'MustBeUsed')
104+
->build(),
105+
];
106+
}
107+
}

0 commit comments

Comments
 (0)