Skip to content

Commit 7ff6c9e

Browse files
dereuromarkclaude
dereuromark
and
claude
authored
Fix EntityPatchRector and newExpr rector issues (#351)
* Fix EntityPatchRector and newExpr rector issues This commit addresses two bugs reported in issue #350: 1. EntityPatchRector now only transforms Entity objects - Added type checking to ensure only Cake\ORM\Entity instances have their set() calls converted to patch() - Previously transformed ANY object's set([array]) to patch([array]) - Now checks object type using ObjectType and isInstanceOf() 2. NewExprToFuncRector handles newExpr()->count() pattern - Created new rector to transform $query->newExpr()->count() to $query->func()->count('*') - This pattern requires func() not expr() as newExpr()->count() creates aggregate functions - Runs before general newExpr → expr rename in both cakephp44 and cakephp53 rulesets to catch this specific pattern first Fixes #350 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Expand NewExprToFuncRector to handle all FunctionsBuilder methods Extended the rector to transform all common FunctionsBuilder method calls, not just count(). Now handles: - Aggregate functions: count(), sum(), avg(), min(), max(), rowNumber(), lag(), lead() - Date/time functions: dateDiff(), datePart(), extract(), dateAdd(), now(), weekday(), dayOfWeek() - Other SQL functions: concat(), coalesce(), cast(), rand(), jsonValue(), aggregate() This ensures that any pattern like $query->newExpr()->sum() is correctly transformed to $query->func()->sum() instead of $query->expr()->sum(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add comprehensive tests for EntityPatchRector and NewExprToFuncRector Added test coverage for both rector rules with multiple test cases: EntityPatchRector tests: - Transforms Entity->set([...]) to Entity->patch([...]) - Skips non-Entity objects (the key bug fix) - Skips set() calls with variables (not array literals) - Skips set() calls with non-array arguments NewExprToFuncRector tests: - Transforms all aggregate functions (count, sum, avg, min, max) - Transforms date/time functions (now, dateDiff, extract) - Transforms other SQL functions (concat, coalesce, rand) - Adds '*' argument to count() when missing - Skips non-FunctionsBuilder methods (eq, gt, and, etc.) - Skips non-Query objects Also fixed NewExprToFuncRector to check for ObjectType before calling isInstanceOf() to prevent ErrorType crashes. All tests pass (42 tests, 123 assertions). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 52e6c45 commit 7ff6c9e

13 files changed

Lines changed: 506 additions & 1 deletion

File tree

‎config/rector/sets/cakephp44.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
<?php
22
declare(strict_types=1);
33

4+
use Cake\Upgrade\Rector\Rector\MethodCall\NewExprToFuncRector;
45
use Rector\Config\RectorConfig;
56
use Rector\Renaming\Rector\MethodCall\RenameMethodRector;
67
use Rector\Renaming\Rector\Name\RenameClassRector;
@@ -16,8 +17,11 @@
1617
'Cake\TestSuite\HttpClientTrait' => 'Cake\Http\TestSuite\HttpClientTrait',
1718
]);
1819

20+
// Apply newExpr()->count() -> func()->count('*') transformation before general newExpr rename
21+
$rectorConfig->rule(NewExprToFuncRector::class);
22+
1923
$rectorConfig->ruleWithConfiguration(
2024
RenameMethodRector::class,
21-
[new MethodCallRename('Cake\Database\Query', 'newExpr', 'expr')]
25+
[new MethodCallRename('Cake\Database\Query', 'newExpr', 'expr')],
2226
);
2327
};

‎config/rector/sets/cakephp53.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,17 @@
33

44
use Cake\Upgrade\Rector\Rector\MethodCall\EntityIsEmptyRector;
55
use Cake\Upgrade\Rector\Rector\MethodCall\EntityPatchRector;
6+
use Cake\Upgrade\Rector\Rector\MethodCall\NewExprToFuncRector;
67
use Rector\Config\RectorConfig;
78
use Rector\Renaming\Rector\MethodCall\RenameMethodRector;
89
use Rector\Renaming\Rector\Name\RenameClassRector;
910
use Rector\Renaming\ValueObject\MethodCallRename;
1011

1112
# @see https://book.cakephp.org/5/en/appendices/5-3-migration-guide.html
1213
return static function (RectorConfig $rectorConfig): void {
14+
// Apply newExpr()->count() -> func()->count('*') transformation before general newExpr rename
15+
$rectorConfig->rule(NewExprToFuncRector::class);
16+
1317
$rectorConfig->ruleWithConfiguration(RenameMethodRector::class, [
1418
new MethodCallRename('Cake\Database\Query', 'newExpr', 'expr'),
1519
]);

‎src/Rector/Rector/MethodCall/EntityPatchRector.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@
33

44
namespace Cake\Upgrade\Rector\Rector\MethodCall;
55

6+
use Cake\ORM\Entity;
67
use PhpParser\Node;
78
use PhpParser\Node\Expr\Array_;
89
use PhpParser\Node\Expr\MethodCall;
910
use PhpParser\Node\Identifier;
11+
use PHPStan\Type\ObjectType;
1012
use Rector\Rector\AbstractRector;
1113
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
1214
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
@@ -59,6 +61,15 @@ public function refactor(Node $node): ?Node
5961
return null;
6062
}
6163

64+
$callerType = $this->getType($node->var);
65+
if (!$callerType instanceof ObjectType) {
66+
return null;
67+
}
68+
69+
if (!$callerType->isInstanceOf(Entity::class)->yes()) {
70+
return null;
71+
}
72+
6273
// change the method name
6374
$node->name = new Identifier('patch');
6475

‎src/Rector/Rector/MethodCall/NewExprToFuncRector.php‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
namespace Cake\Upgrade\Rector\Rector\MethodCall;
5+
6+
use PhpParser\Node;
7+
use PhpParser\Node\Arg;
8+
use PhpParser\Node\Expr\MethodCall;
9+
use PhpParser\Node\Identifier;
10+
use PhpParser\Node\Scalar\String_;
11+
use PHPStan\Type\ObjectType;
12+
use Rector\Rector\AbstractRector;
13+
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
14+
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
15+
16+
/**
17+
* Transforms $query->newExpr()->aggregate() to $query->func()->aggregate()
18+
*
19+
* newExpr() was used to create expression builders, but when followed by aggregate
20+
* or SQL function calls, it should use func() instead which is the function builder.
21+
*
22+
* Handles common FunctionsBuilder methods like:
23+
* - Aggregates: count(), sum(), avg(), min(), max(), rowNumber(), lag(), lead()
24+
* - Date functions: dateDiff(), datePart(), extract(), dateAdd(), now()
25+
* - Other functions: concat(), coalesce(), cast(), rand()
26+
*/
27+
final class NewExprToFuncRector extends AbstractRector
28+
{
29+
/**
30+
* List of FunctionsBuilder methods that should trigger the transformation
31+
*/
32+
private const FUNC_BUILDER_METHODS = [
33+
// Aggregate functions
34+
'count',
35+
'sum',
36+
'avg',
37+
'min',
38+
'max',
39+
'rowNumber',
40+
'lag',
41+
'lead',
42+
// Date/time functions
43+
'dateDiff',
44+
'datePart',
45+
'extract',
46+
'dateAdd',
47+
'now',
48+
'weekday',
49+
'dayOfWeek',
50+
// Other SQL functions
51+
'concat',
52+
'coalesce',
53+
'cast',
54+
'rand',
55+
'jsonValue',
56+
'aggregate',
57+
];
58+
59+
public function getRuleDefinition(): RuleDefinition
60+
{
61+
return new RuleDefinition(
62+
'Change $query->newExpr()->funcMethod() to $query->func()->funcMethod() for FunctionsBuilder methods',
63+
[
64+
new CodeSample(
65+
<<<'CODE_SAMPLE'
66+
$query->newExpr()->count();
67+
$query->newExpr()->sum('total');
68+
$query->newExpr()->avg('score');
69+
CODE_SAMPLE
70+
,
71+
<<<'CODE_SAMPLE'
72+
$query->func()->count('*');
73+
$query->func()->sum('total');
74+
$query->func()->avg('score');
75+
CODE_SAMPLE,
76+
),
77+
],
78+
);
79+
}
80+
81+
public function getNodeTypes(): array
82+
{
83+
return [MethodCall::class];
84+
}
85+
86+
public function refactor(Node $node): ?Node
87+
{
88+
if (!$node instanceof MethodCall) {
89+
return null;
90+
}
91+
92+
// Check if this is a FunctionsBuilder method call
93+
if (!$node->name instanceof Identifier) {
94+
return null;
95+
}
96+
97+
$methodName = $node->name->toString();
98+
if (!in_array($methodName, self::FUNC_BUILDER_METHODS, true)) {
99+
return null;
100+
}
101+
102+
// Check if the var is also a MethodCall (chained call)
103+
if (!$node->var instanceof MethodCall) {
104+
return null;
105+
}
106+
107+
$innerMethodCall = $node->var;
108+
109+
// Check if the inner call is ->newExpr()
110+
if (!$innerMethodCall->name instanceof Identifier || $innerMethodCall->name->toString() !== 'newExpr') {
111+
return null;
112+
}
113+
114+
// Check if the caller is a Query object (Cake\Database\Query or similar)
115+
$callerType = $this->getType($innerMethodCall->var);
116+
if (!$callerType instanceof ObjectType) {
117+
return null;
118+
}
119+
120+
if (
121+
!$callerType->isInstanceOf('Cake\Database\Query')->yes() &&
122+
!$callerType->isInstanceOf('Cake\ORM\Query')->yes()
123+
) {
124+
return null;
125+
}
126+
127+
// Change newExpr to func
128+
$innerMethodCall->name = new Identifier('func');
129+
130+
// Add '*' argument to count() if it doesn't have arguments
131+
if ($methodName === 'count' && empty($node->args)) {
132+
$node->args = [new Arg(new String_('*'))];
133+
}
134+
135+
return $node;
136+
}
137+
}

‎tests/TestCase/Rector/MethodCall/EntityPatchRector/EntityPatchRectorTest.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
namespace Cake\Upgrade\Test\TestCase\Rector\MethodCall\EntityPatchRector;
5+
6+
use Iterator;
7+
use Rector\Testing\PHPUnit\AbstractRectorTestCase;
8+
9+
final class EntityPatchRectorTest extends AbstractRectorTestCase
10+
{
11+
/**
12+
* @dataProvider provideData()
13+
*/
14+
public function test(string $filePath): void
15+
{
16+
$this->doTestFile($filePath);
17+
}
18+
19+
public static function provideData(): Iterator
20+
{
21+
return self::yieldFilesFromDirectory(__DIR__ . '/Fixture');
22+
}
23+
24+
public function provideConfigFilePath(): string
25+
{
26+
return __DIR__ . '/config/configured_rule.php';
27+
}
28+
}

‎tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/fixture.php.inc‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<?php
2+
3+
namespace Cake\Upgrade\Test\TestCase\Rector\MethodCall\EntityPatchRector\Fixture;
4+
5+
use Cake\ORM\Entity;
6+
7+
class MyEntity extends Entity
8+
{
9+
}
10+
11+
class UserController
12+
{
13+
public function edit()
14+
{
15+
$entity = new MyEntity();
16+
17+
// Should transform: Entity->set with array literal
18+
$entity->set(['name' => 'Test', 'email' => 'test@example.com']);
19+
20+
// Should NOT transform: set with variable
21+
$data = ['name' => 'Test'];
22+
$entity->set($data);
23+
24+
// Should NOT transform: set with non-array argument
25+
$entity->set('name', 'Test');
26+
27+
return $entity;
28+
}
29+
}
30+
31+
?>
32+
-----
33+
<?php
34+
35+
namespace Cake\Upgrade\Test\TestCase\Rector\MethodCall\EntityPatchRector\Fixture;
36+
37+
use Cake\ORM\Entity;
38+
39+
class MyEntity extends Entity
40+
{
41+
}
42+
43+
class UserController
44+
{
45+
public function edit()
46+
{
47+
$entity = new MyEntity();
48+
49+
// Should transform: Entity->set with array literal
50+
$entity->patch(['name' => 'Test', 'email' => 'test@example.com']);
51+
52+
// Should NOT transform: set with variable
53+
$data = ['name' => 'Test'];
54+
$entity->set($data);
55+
56+
// Should NOT transform: set with non-array argument
57+
$entity->set('name', 'Test');
58+
59+
return $entity;
60+
}
61+
}
62+
63+
?>

‎tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/skip_non_entity.php.inc‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php
2+
3+
namespace Cake\Upgrade\Test\TestCase\Rector\MethodCall\EntityPatchRector\Fixture;
4+
5+
class SomeOtherClass
6+
{
7+
public function set(array $data): void
8+
{
9+
// Do something
10+
}
11+
}
12+
13+
class MyController
14+
{
15+
public function process()
16+
{
17+
$object = new SomeOtherClass();
18+
19+
// Should NOT transform: not an Entity
20+
$object->set(['key' => 'value']);
21+
22+
// Should NOT transform: unknown object type
23+
$someObject->set(['data' => 'test']);
24+
25+
return $object;
26+
}
27+
}
28+
29+
?>
30+
-----
31+
<?php
32+
33+
namespace Cake\Upgrade\Test\TestCase\Rector\MethodCall\EntityPatchRector\Fixture;
34+
35+
class SomeOtherClass
36+
{
37+
public function set(array $data): void
38+
{
39+
// Do something
40+
}
41+
}
42+
43+
class MyController
44+
{
45+
public function process()
46+
{
47+
$object = new SomeOtherClass();
48+
49+
// Should NOT transform: not an Entity
50+
$object->set(['key' => 'value']);
51+
52+
// Should NOT transform: unknown object type
53+
$someObject->set(['data' => 'test']);
54+
55+
return $object;
56+
}
57+
}
58+
59+
?>

‎tests/TestCase/Rector/MethodCall/EntityPatchRector/config/configured_rule.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
use Cake\Upgrade\Rector\Rector\MethodCall\EntityPatchRector;
5+
use Rector\Config\RectorConfig;
6+
7+
return static function (RectorConfig $rectorConfig): void {
8+
$rectorConfig->rule(EntityPatchRector::class);
9+
};

0 commit comments

Comments
 (0)