Fix EntityPatchRector and newExpr rector issues #351
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
Contributor
|
please add tests like we have for all the other custom rector rules |
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>
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>
Member
Author
|
Should be all done. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes two bugs reported in #350:
Bug 1: EntityPatchRector was overly broad
->set([array])to->patch([array])without checking object typeCake\ORM\EntityinstancesObjectTypecheck andisInstanceOf(Entity::class)validationBug 2: newExpr()->count() pattern not handled correctly
$query->newExpr()->count()was being transformed to$query->expr()->count()which is invalid$query->func()->count('*')since it's creating aggregate functionsNewExprToFuncRectorthat runs before the general newExpr → expr renameTest plan
Related Issues
Fixes #350
🤖 Generated with Claude Code