From a88214e668b62c400d715266ed0c12a9c32df490 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 10 Nov 2025 16:19:30 +0100 Subject: [PATCH 1/3] Fix EntityPatchRector and newExpr rector issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- config/rector/sets/cakephp44.php | 6 +- config/rector/sets/cakephp53.php | 4 + .../Rector/MethodCall/EntityPatchRector.php | 11 +++ .../Rector/MethodCall/NewExprToFuncRector.php | 88 +++++++++++++++++++ 4 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 src/Rector/Rector/MethodCall/NewExprToFuncRector.php diff --git a/config/rector/sets/cakephp44.php b/config/rector/sets/cakephp44.php index 7987ce71..2c495251 100644 --- a/config/rector/sets/cakephp44.php +++ b/config/rector/sets/cakephp44.php @@ -1,6 +1,7 @@ 'Cake\Http\TestSuite\HttpClientTrait', ]); + // Apply newExpr()->count() -> func()->count('*') transformation before general newExpr rename + $rectorConfig->rule(NewExprToFuncRector::class); + $rectorConfig->ruleWithConfiguration( RenameMethodRector::class, - [new MethodCallRename('Cake\Database\Query', 'newExpr', 'expr')] + [new MethodCallRename('Cake\Database\Query', 'newExpr', 'expr')], ); }; diff --git a/config/rector/sets/cakephp53.php b/config/rector/sets/cakephp53.php index 4987fbbe..ec824bcd 100644 --- a/config/rector/sets/cakephp53.php +++ b/config/rector/sets/cakephp53.php @@ -3,6 +3,7 @@ use Cake\Upgrade\Rector\Rector\MethodCall\EntityIsEmptyRector; use Cake\Upgrade\Rector\Rector\MethodCall\EntityPatchRector; +use Cake\Upgrade\Rector\Rector\MethodCall\NewExprToFuncRector; use Rector\Config\RectorConfig; use Rector\Renaming\Rector\MethodCall\RenameMethodRector; use Rector\Renaming\Rector\Name\RenameClassRector; @@ -10,6 +11,9 @@ # @see https://book.cakephp.org/5/en/appendices/5-3-migration-guide.html return static function (RectorConfig $rectorConfig): void { + // Apply newExpr()->count() -> func()->count('*') transformation before general newExpr rename + $rectorConfig->rule(NewExprToFuncRector::class); + $rectorConfig->ruleWithConfiguration(RenameMethodRector::class, [ new MethodCallRename('Cake\Database\Query', 'newExpr', 'expr'), ]); diff --git a/src/Rector/Rector/MethodCall/EntityPatchRector.php b/src/Rector/Rector/MethodCall/EntityPatchRector.php index 141d221c..a5c6af0b 100644 --- a/src/Rector/Rector/MethodCall/EntityPatchRector.php +++ b/src/Rector/Rector/MethodCall/EntityPatchRector.php @@ -3,10 +3,12 @@ namespace Cake\Upgrade\Rector\Rector\MethodCall; +use Cake\ORM\Entity; use PhpParser\Node; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Identifier; +use PHPStan\Type\ObjectType; use Rector\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -59,6 +61,15 @@ public function refactor(Node $node): ?Node return null; } + $callerType = $this->getType($node->var); + if (!$callerType instanceof ObjectType) { + return null; + } + + if (!$callerType->isInstanceOf(Entity::class)->yes()) { + return null; + } + // change the method name $node->name = new Identifier('patch'); diff --git a/src/Rector/Rector/MethodCall/NewExprToFuncRector.php b/src/Rector/Rector/MethodCall/NewExprToFuncRector.php new file mode 100644 index 00000000..c560bc2a --- /dev/null +++ b/src/Rector/Rector/MethodCall/NewExprToFuncRector.php @@ -0,0 +1,88 @@ +newExpr()->count() to $query->func()->count('*') + * + * newExpr() was used to create expression builders, but when followed by count(), + * it should use func() instead which is the aggregate function builder. + */ +final class NewExprToFuncRector extends AbstractRector +{ + public function getRuleDefinition(): RuleDefinition + { + return new RuleDefinition( + 'Change $query->newExpr()->count() to $query->func()->count(\'*\')', + [ + new CodeSample( + <<<'CODE_SAMPLE' +$query->newExpr()->count(); +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +$query->func()->count('*'); +CODE_SAMPLE, + ), + ], + ); + } + + public function getNodeTypes(): array + { + return [MethodCall::class]; + } + + public function refactor(Node $node): ?Node + { + if (!$node instanceof MethodCall) { + return null; + } + + // Check if this is a ->count() call + if (!$node->name instanceof Identifier || $node->name->toString() !== 'count') { + return null; + } + + // Check if the var is also a MethodCall (chained call) + if (!$node->var instanceof MethodCall) { + return null; + } + + $innerMethodCall = $node->var; + + // Check if the inner call is ->newExpr() + if (!$innerMethodCall->name instanceof Identifier || $innerMethodCall->name->toString() !== 'newExpr') { + return null; + } + + // Check if the caller is a Query object (Cake\Database\Query or similar) + $callerType = $this->getType($innerMethodCall->var); + if ( + !$callerType->isInstanceOf('Cake\Database\Query')->yes() && + !$callerType->isInstanceOf('Cake\ORM\Query')->yes() + ) { + return null; + } + + // Change newExpr to func + $innerMethodCall->name = new Identifier('func'); + + // Add '*' argument to count() if it doesn't have arguments + if (empty($node->args)) { + $node->args = [new Arg(new String_('*'))]; + } + + return $node; + } +} From d3bd5c2f2f8ff7b0cfd7e35b39e4fb99d8ef3f7c Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 10 Nov 2025 16:24:15 +0100 Subject: [PATCH 2/3] Expand NewExprToFuncRector to handle all FunctionsBuilder methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Rector/MethodCall/NewExprToFuncRector.php | 58 ++++++++++++++++--- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/src/Rector/Rector/MethodCall/NewExprToFuncRector.php b/src/Rector/Rector/MethodCall/NewExprToFuncRector.php index c560bc2a..db001628 100644 --- a/src/Rector/Rector/MethodCall/NewExprToFuncRector.php +++ b/src/Rector/Rector/MethodCall/NewExprToFuncRector.php @@ -13,25 +13,64 @@ use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; /** - * Transforms $query->newExpr()->count() to $query->func()->count('*') + * Transforms $query->newExpr()->aggregate() to $query->func()->aggregate() * - * newExpr() was used to create expression builders, but when followed by count(), - * it should use func() instead which is the aggregate function builder. + * newExpr() was used to create expression builders, but when followed by aggregate + * or SQL function calls, it should use func() instead which is the function builder. + * + * Handles common FunctionsBuilder methods like: + * - Aggregates: count(), sum(), avg(), min(), max(), rowNumber(), lag(), lead() + * - Date functions: dateDiff(), datePart(), extract(), dateAdd(), now() + * - Other functions: concat(), coalesce(), cast(), rand() */ final class NewExprToFuncRector extends AbstractRector { + /** + * List of FunctionsBuilder methods that should trigger the transformation + */ + private const FUNC_BUILDER_METHODS = [ + // 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', + ]; + public function getRuleDefinition(): RuleDefinition { return new RuleDefinition( - 'Change $query->newExpr()->count() to $query->func()->count(\'*\')', + 'Change $query->newExpr()->funcMethod() to $query->func()->funcMethod() for FunctionsBuilder methods', [ new CodeSample( <<<'CODE_SAMPLE' $query->newExpr()->count(); +$query->newExpr()->sum('total'); +$query->newExpr()->avg('score'); CODE_SAMPLE , <<<'CODE_SAMPLE' $query->func()->count('*'); +$query->func()->sum('total'); +$query->func()->avg('score'); CODE_SAMPLE, ), ], @@ -49,8 +88,13 @@ public function refactor(Node $node): ?Node return null; } - // Check if this is a ->count() call - if (!$node->name instanceof Identifier || $node->name->toString() !== 'count') { + // Check if this is a FunctionsBuilder method call + if (!$node->name instanceof Identifier) { + return null; + } + + $methodName = $node->name->toString(); + if (!in_array($methodName, self::FUNC_BUILDER_METHODS, true)) { return null; } @@ -79,7 +123,7 @@ public function refactor(Node $node): ?Node $innerMethodCall->name = new Identifier('func'); // Add '*' argument to count() if it doesn't have arguments - if (empty($node->args)) { + if ($methodName === 'count' && empty($node->args)) { $node->args = [new Arg(new String_('*'))]; } From 10f92560319a5a9c7b07b34aa7c81425d594ed78 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 10 Nov 2025 16:28:37 +0100 Subject: [PATCH 3/3] Add comprehensive tests for EntityPatchRector and NewExprToFuncRector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Rector/MethodCall/NewExprToFuncRector.php | 5 ++ .../EntityPatchRectorTest.php | 28 +++++++++ .../EntityPatchRector/Fixture/fixture.php.inc | 63 +++++++++++++++++++ .../Fixture/skip_non_entity.php.inc | 59 +++++++++++++++++ .../config/configured_rule.php | 9 +++ .../Fixture/aggregate_functions.php.inc | 49 +++++++++++++++ .../Fixture/skip_non_func_methods.php.inc | 53 ++++++++++++++++ .../Fixture/sql_functions.php.inc | 51 +++++++++++++++ .../NewExprToFuncRectorTest.php | 28 +++++++++ .../config/configured_rule.php | 9 +++ 10 files changed, 354 insertions(+) create mode 100644 tests/TestCase/Rector/MethodCall/EntityPatchRector/EntityPatchRectorTest.php create mode 100644 tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/fixture.php.inc create mode 100644 tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/skip_non_entity.php.inc create mode 100644 tests/TestCase/Rector/MethodCall/EntityPatchRector/config/configured_rule.php create mode 100644 tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/aggregate_functions.php.inc create mode 100644 tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/skip_non_func_methods.php.inc create mode 100644 tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/sql_functions.php.inc create mode 100644 tests/TestCase/Rector/MethodCall/NewExprToFuncRector/NewExprToFuncRectorTest.php create mode 100644 tests/TestCase/Rector/MethodCall/NewExprToFuncRector/config/configured_rule.php diff --git a/src/Rector/Rector/MethodCall/NewExprToFuncRector.php b/src/Rector/Rector/MethodCall/NewExprToFuncRector.php index db001628..7ca15ea9 100644 --- a/src/Rector/Rector/MethodCall/NewExprToFuncRector.php +++ b/src/Rector/Rector/MethodCall/NewExprToFuncRector.php @@ -8,6 +8,7 @@ use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Identifier; use PhpParser\Node\Scalar\String_; +use PHPStan\Type\ObjectType; use Rector\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -112,6 +113,10 @@ public function refactor(Node $node): ?Node // Check if the caller is a Query object (Cake\Database\Query or similar) $callerType = $this->getType($innerMethodCall->var); + if (!$callerType instanceof ObjectType) { + return null; + } + if ( !$callerType->isInstanceOf('Cake\Database\Query')->yes() && !$callerType->isInstanceOf('Cake\ORM\Query')->yes() diff --git a/tests/TestCase/Rector/MethodCall/EntityPatchRector/EntityPatchRectorTest.php b/tests/TestCase/Rector/MethodCall/EntityPatchRector/EntityPatchRectorTest.php new file mode 100644 index 00000000..b063f92d --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/EntityPatchRector/EntityPatchRectorTest.php @@ -0,0 +1,28 @@ +doTestFile($filePath); + } + + public static function provideData(): Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/fixture.php.inc b/tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/fixture.php.inc new file mode 100644 index 00000000..6ec39036 --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/fixture.php.inc @@ -0,0 +1,63 @@ +set with array literal + $entity->set(['name' => 'Test', 'email' => 'test@example.com']); + + // Should NOT transform: set with variable + $data = ['name' => 'Test']; + $entity->set($data); + + // Should NOT transform: set with non-array argument + $entity->set('name', 'Test'); + + return $entity; + } +} + +?> +----- +set with array literal + $entity->patch(['name' => 'Test', 'email' => 'test@example.com']); + + // Should NOT transform: set with variable + $data = ['name' => 'Test']; + $entity->set($data); + + // Should NOT transform: set with non-array argument + $entity->set('name', 'Test'); + + return $entity; + } +} + +?> diff --git a/tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/skip_non_entity.php.inc b/tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/skip_non_entity.php.inc new file mode 100644 index 00000000..e45cc18e --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/EntityPatchRector/Fixture/skip_non_entity.php.inc @@ -0,0 +1,59 @@ +set(['key' => 'value']); + + // Should NOT transform: unknown object type + $someObject->set(['data' => 'test']); + + return $object; + } +} + +?> +----- +set(['key' => 'value']); + + // Should NOT transform: unknown object type + $someObject->set(['data' => 'test']); + + return $object; + } +} + +?> diff --git a/tests/TestCase/Rector/MethodCall/EntityPatchRector/config/configured_rule.php b/tests/TestCase/Rector/MethodCall/EntityPatchRector/config/configured_rule.php new file mode 100644 index 00000000..3bdc46cb --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/EntityPatchRector/config/configured_rule.php @@ -0,0 +1,9 @@ +rule(EntityPatchRector::class); +}; diff --git a/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/aggregate_functions.php.inc b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/aggregate_functions.php.inc new file mode 100644 index 00000000..6f5bbf63 --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/aggregate_functions.php.inc @@ -0,0 +1,49 @@ +newExpr()->count(); + $countField = $query->newExpr()->count('id'); + $sum = $query->newExpr()->sum('total'); + $avg = $query->newExpr()->avg('score'); + $min = $query->newExpr()->min('price'); + $max = $query->newExpr()->max('price'); + + return compact('count', 'sum', 'avg', 'min', 'max'); + } +} + +?> +----- +func()->count('*'); + $countField = $query->func()->count('id'); + $sum = $query->func()->sum('total'); + $avg = $query->func()->avg('score'); + $min = $query->func()->min('price'); + $max = $query->func()->max('price'); + + return compact('count', 'sum', 'avg', 'min', 'max'); + } +} + +?> diff --git a/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/skip_non_func_methods.php.inc b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/skip_non_func_methods.php.inc new file mode 100644 index 00000000..27dc1fc1 --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/skip_non_func_methods.php.inc @@ -0,0 +1,53 @@ +newExpr()->eq('status', 1); + $expr2 = $query->newExpr()->gt('price', 100); + $expr3 = $query->newExpr()->and(['status' => 1, 'active' => true]); + + // Should NOT transform: standalone newExpr() call + $expr4 = $query->newExpr(); + + // Should NOT transform: non-Query object + $someObject->newExpr()->count(); + + return $query; + } +} + +?> +----- +newExpr()->eq('status', 1); + $expr2 = $query->newExpr()->gt('price', 100); + $expr3 = $query->newExpr()->and(['status' => 1, 'active' => true]); + + // Should NOT transform: standalone newExpr() call + $expr4 = $query->newExpr(); + + // Should NOT transform: non-Query object + $someObject->newExpr()->count(); + + return $query; + } +} + +?> diff --git a/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/sql_functions.php.inc b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/sql_functions.php.inc new file mode 100644 index 00000000..ab04a5f1 --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/Fixture/sql_functions.php.inc @@ -0,0 +1,51 @@ +newExpr()->now(); + $diff = $query->newExpr()->dateDiff('created', 'modified'); + $extract = $query->newExpr()->extract('year', 'created'); + + // Other SQL functions + $concat = $query->newExpr()->concat(['first_name', 'last_name']); + $coalesce = $query->newExpr()->coalesce(['email', 'backup_email']); + $rand = $query->newExpr()->rand(); + + return $query; + } +} + +?> +----- +func()->now(); + $diff = $query->func()->dateDiff('created', 'modified'); + $extract = $query->func()->extract('year', 'created'); + + // Other SQL functions + $concat = $query->func()->concat(['first_name', 'last_name']); + $coalesce = $query->func()->coalesce(['email', 'backup_email']); + $rand = $query->func()->rand(); + + return $query; + } +} + +?> diff --git a/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/NewExprToFuncRectorTest.php b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/NewExprToFuncRectorTest.php new file mode 100644 index 00000000..86812521 --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/NewExprToFuncRectorTest.php @@ -0,0 +1,28 @@ +doTestFile($filePath); + } + + public static function provideData(): Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/config/configured_rule.php b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/config/configured_rule.php new file mode 100644 index 00000000..e6c2965e --- /dev/null +++ b/tests/TestCase/Rector/MethodCall/NewExprToFuncRector/config/configured_rule.php @@ -0,0 +1,9 @@ +rule(NewExprToFuncRector::class); +};