From ba420d9a6d5a3883c8dc03d5f3f508acecac940f Mon Sep 17 00:00:00 2001 From: Liam Hammett Date: Sat, 4 Oct 2025 18:05:54 +0200 Subject: [PATCH 1/4] Add ModelComparisonToIsMethodRector --- docs/rector_rules_overview.md | 15 ++ .../Expr/ModelComparisonToIsMethodRector.php | 156 ++++++++++++++++++ .../Fixture/complex_relationships.php.inc | 89 ++++++++++ .../Fixture/fixture.php.inc | 85 ++++++++++ .../Fixture/multiple_relationships.php.inc | 103 ++++++++++++ .../Fixture/skip_invalid_patterns.php.inc | 49 ++++++ .../Fixture/skip_non_model_variables.php.inc | 61 +++++++ .../ModelComparisonToIsMethodRectorTest.php | 31 ++++ .../config/configured_rule.php | 12 ++ 9 files changed, 601 insertions(+) create mode 100644 src/Rector/Expr/ModelComparisonToIsMethodRector.php create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/complex_relationships.php.inc create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/fixture.php.inc create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/multiple_relationships.php.inc create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_invalid_patterns.php.inc create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_non_model_variables.php.inc create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/ModelComparisonToIsMethodRectorTest.php create mode 100644 tests/Rector/Expr/ModelComparisonToIsMethodRector/config/configured_rule.php diff --git a/docs/rector_rules_overview.md b/docs/rector_rules_overview.md index 9c9c85011..5757445b4 100644 --- a/docs/rector_rules_overview.md +++ b/docs/rector_rules_overview.md @@ -1020,6 +1020,21 @@ Refactor Model `$casts` property with `casts()` method
+## ModelComparisonToIsMethodRector + +Convert model ID comparisons to use the `is()` method + +- class: [`RectorLaravel\Rector\Expr\ModelComparisonToIsMethodRector`](../src/Rector/Expr/ModelComparisonToIsMethodRector.php) + +```diff +-$team->user_id === $user->id; +-$post->author_id === $author->id; ++$team->user()->is($user); ++$post->author()->is($author); +``` + +
+ ## NotFilledBlankFuncCallToBlankFilledFuncCallRector Swap the use of NotBooleans used with `filled()` and `blank()` to the correct helper. diff --git a/src/Rector/Expr/ModelComparisonToIsMethodRector.php b/src/Rector/Expr/ModelComparisonToIsMethodRector.php new file mode 100644 index 000000000..6a409ad9d --- /dev/null +++ b/src/Rector/Expr/ModelComparisonToIsMethodRector.php @@ -0,0 +1,156 @@ +user_id === $user->id; +$post->author_id === $author->id; +CODE_SAMPLE, + <<<'CODE_SAMPLE' +$team->user()->is($user); +$post->author()->is($author); +CODE_SAMPLE + )] + ); + } + + /** + * @return array> + */ + public function getNodeTypes(): array + { + return [Equal::class, Identical::class]; + } + + /** + * @param Equal|Identical $node + */ + public function refactor(Node $node): ?Node + { + if (!$node instanceof Equal && !$node instanceof Identical) { + return null; + } + + $result = $this->matchModelComparison($node); + if ($result === null) { + return null; + } + + [$leftVar, $relationshipName, $rightVar] = $result; + + // Enhanced safety check: verify both variables could be models + if (!$this->couldBeModel($leftVar) || !$this->couldBeModel($rightVar)) { + return null; + } + + $relationshipCall = new MethodCall($leftVar, new Identifier($relationshipName)); + $isCall = new MethodCall($relationshipCall, new Identifier('is'), [new Arg($rightVar)]); + + return $isCall; + } + + /** + * @return array{Expr, string, Expr}|null + */ + private function matchModelComparison(Equal|Identical $node): ?array + { + $left = $node->left; + $right = $node->right; + + if (!$left instanceof PropertyFetch || !$right instanceof PropertyFetch) { + return null; + } + + $leftProperty = $left->name; + $rightProperty = $right->name; + + if (!$leftProperty instanceof Identifier || !$rightProperty instanceof Identifier) { + return null; + } + + $leftPropertyName = $leftProperty->name; + $rightPropertyName = $rightProperty->name; + + if ($this->isForeignKeyToIdPattern($leftPropertyName, $rightPropertyName)) { + // $model->foreign_key_id == $otherModel->id + $relationshipName = $this->extractRelationshipName($leftPropertyName); + return [$left->var, $relationshipName, $right->var]; + } + + if ($this->isForeignKeyToIdPattern($rightPropertyName, $leftPropertyName)) { + // $otherModel->id == $model->foreign_key_id + $relationshipName = $this->extractRelationshipName($rightPropertyName); + return [$right->var, $relationshipName, $left->var]; + } + + return null; + } + + private function isForeignKeyToIdPattern(string $leftProperty, string $rightProperty): bool + { + return str_ends_with($leftProperty, '_id') && $rightProperty === 'id'; + } + + private function extractRelationshipName(string $foreignKeyProperty): string + { + return substr($foreignKeyProperty, 0, -3); + } + + private function couldBeModel(Expr $expr): bool + { + $modelType = new ObjectType('Illuminate\Database\Eloquent\Model'); + + // For property fetch expressions like $user->team_id, check the variable part ($user) + if ($expr instanceof PropertyFetch) { + if ($this->isObjectType($expr->var, $modelType)) { + return true; + } + } + + // For variable expressions like $user, check the variable directly + if ($expr instanceof Variable) { + if ($this->isObjectType($expr, $modelType)) { + return true; + } + } + + // Fallback: check for obvious non-model patterns + if ($expr instanceof Variable && is_string($expr->name)) { + $varName = $expr->name; + // Skip variables that are obviously not models + $nonModelNames = ['i', 'j', 'k', 'x', 'y', 'z', 'count', 'index', 'key', 'value', 'data', 'result', 'item', 'element']; + if (in_array($varName, $nonModelNames, true)) { + return false; + } + } + + // Allow by default for backwards compatibility - be permissive unless obviously not a model + return true; + } +} diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/complex_relationships.php.inc b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/complex_relationships.php.inc new file mode 100644 index 000000000..866811c27 --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/complex_relationships.php.inc @@ -0,0 +1,89 @@ +belongsTo(Post::class); + } + + public function author() + { + return $this->belongsTo(User::class); + } +} + +class Post extends Model +{ + public $id; + public $author_id; + + public function author() + { + return $this->belongsTo(User::class); + } +} + +class User extends Model +{ + public $id; +} + +// Complex relationship comparisons +$comment->post_id === $post->id; +$comment->author_id === $author->id; +$post->author_id === $user->id; + +?> +----- +belongsTo(Post::class); + } + + public function author() + { + return $this->belongsTo(User::class); + } +} + +class Post extends Model +{ + public $id; + public $author_id; + + public function author() + { + return $this->belongsTo(User::class); + } +} + +class User extends Model +{ + public $id; +} + +// Complex relationship comparisons +$comment->post()->is($post); +$comment->author()->is($author); +$post->author()->is($user); + +?> diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/fixture.php.inc b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/fixture.php.inc new file mode 100644 index 000000000..32747c84d --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/fixture.php.inc @@ -0,0 +1,85 @@ +belongsTo(User::class); + } +} + +class Post extends Model +{ + public $author_id; + + public function author() + { + return $this->belongsTo(User::class); + } +} + +// Basic comparison cases +$team->user_id === $user->id; +$team->user_id === $user->id; +$post->author_id === $author->id; +$post->author_id === $author->id; + +// Reversed comparison +$user->id === $team->user_id; +$author->id === $post->author_id; + +?> +----- +belongsTo(User::class); + } +} + +class Post extends Model +{ + public $author_id; + + public function author() + { + return $this->belongsTo(User::class); + } +} + +// Basic comparison cases +$team->user()->is($user); +$team->user()->is($user); +$post->author()->is($author); +$post->author()->is($author); + +// Reversed comparison +$team->user()->is($user); +$post->author()->is($author); + +?> diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/multiple_relationships.php.inc b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/multiple_relationships.php.inc new file mode 100644 index 000000000..d3d47facd --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/multiple_relationships.php.inc @@ -0,0 +1,103 @@ +belongsTo(User::class); + } + + public function editor() + { + return $this->belongsTo(User::class); + } +} + +class Comment +{ + public $post_id; + public $author_id; + + public function post() + { + return $this->belongsTo(Post::class); + } + + public function author() + { + return $this->belongsTo(User::class); + } +} + +// Multiple relationships in one function +function checkOwnership($post, $comment, $user) { + if ($post->author_id === $user->id && $comment->author_id === $user->id) { + return true; + } + + return $post->editor_id === $user->id || $comment->post_id === $post->id; +} + +?> +----- +belongsTo(User::class); + } + + public function editor() + { + return $this->belongsTo(User::class); + } +} + +class Comment +{ + public $post_id; + public $author_id; + + public function post() + { + return $this->belongsTo(Post::class); + } + + public function author() + { + return $this->belongsTo(User::class); + } +} + +// Multiple relationships in one function +function checkOwnership($post, $comment, $user) { + if ($post->author()->is($user) && $comment->author()->is($user)) { + return true; + } + + return $post->editor()->is($user) || $comment->post()->is($post); +} + +?> diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_invalid_patterns.php.inc b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_invalid_patterns.php.inc new file mode 100644 index 000000000..a5b89cfc4 --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_invalid_patterns.php.inc @@ -0,0 +1,49 @@ +name === $user->email; +$team->user_id === $user->email; +$team->name === $user->id; +$someVar === $user->id; +$team->user_id === $someVar; + +?> +----- +name === $user->email; +$team->user_id === $user->email; +$team->name === $user->id; +$someVar === $user->id; +$team->user_id === $someVar; + +?> diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_non_model_variables.php.inc b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_non_model_variables.php.inc new file mode 100644 index 000000000..0266eefa8 --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/Fixture/skip_non_model_variables.php.inc @@ -0,0 +1,61 @@ +belongsTo(User::class); + } +} + +// Should NOT be transformed - non-model variables +$team = new Team(); +$user = new User(); +$index = 0; +$data = ['key' => 'value']; + +$team->user_id = $user->id; +$team->user_id === $index; +$post->author_id == $data; + +?> +----- +belongsTo(User::class); + } +} + +// Should NOT be transformed - non-model variables +$team = new Team(); +$user = new User(); +$index = 0; +$data = ['key' => 'value']; + +$team->user_id = $user->id; +$team->user_id === $index; +$post->author_id == $data; + +?> diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/ModelComparisonToIsMethodRectorTest.php b/tests/Rector/Expr/ModelComparisonToIsMethodRector/ModelComparisonToIsMethodRectorTest.php new file mode 100644 index 000000000..9f9066c64 --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/ModelComparisonToIsMethodRectorTest.php @@ -0,0 +1,31 @@ +doTestFile($filePath); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/tests/Rector/Expr/ModelComparisonToIsMethodRector/config/configured_rule.php b/tests/Rector/Expr/ModelComparisonToIsMethodRector/config/configured_rule.php new file mode 100644 index 000000000..35e94ad69 --- /dev/null +++ b/tests/Rector/Expr/ModelComparisonToIsMethodRector/config/configured_rule.php @@ -0,0 +1,12 @@ +import(__DIR__ . '/../../../../../config/config.php'); + + $rectorConfig->rule(ModelComparisonToIsMethodRector::class); +}; From 7b719540fc3245c8ade5097739781250c3ca7dcd Mon Sep 17 00:00:00 2001 From: Liam Hammett Date: Mon, 6 Oct 2025 13:10:22 +0200 Subject: [PATCH 2/4] Code style fixes --- .../Expr/ModelComparisonToIsMethodRector.php | 45 +++++++------------ 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/src/Rector/Expr/ModelComparisonToIsMethodRector.php b/src/Rector/Expr/ModelComparisonToIsMethodRector.php index 6a409ad9d..96682da96 100644 --- a/src/Rector/Expr/ModelComparisonToIsMethodRector.php +++ b/src/Rector/Expr/ModelComparisonToIsMethodRector.php @@ -49,11 +49,11 @@ public function getNodeTypes(): array } /** - * @param Equal|Identical $node + * @param Equal|Identical $node */ public function refactor(Node $node): ?Node { - if (!$node instanceof Equal && !$node instanceof Identical) { + if (! $node instanceof Equal && ! $node instanceof Identical) { return null; } @@ -65,14 +65,13 @@ public function refactor(Node $node): ?Node [$leftVar, $relationshipName, $rightVar] = $result; // Enhanced safety check: verify both variables could be models - if (!$this->couldBeModel($leftVar) || !$this->couldBeModel($rightVar)) { + if (! $this->couldBeModel($leftVar) || ! $this->couldBeModel($rightVar)) { return null; } - $relationshipCall = new MethodCall($leftVar, new Identifier($relationshipName)); - $isCall = new MethodCall($relationshipCall, new Identifier('is'), [new Arg($rightVar)]); + $methodCall = new MethodCall($leftVar, new Identifier($relationshipName)); - return $isCall; + return new MethodCall($methodCall, new Identifier('is'), [new Arg($rightVar)]); } /** @@ -83,14 +82,14 @@ private function matchModelComparison(Equal|Identical $node): ?array $left = $node->left; $right = $node->right; - if (!$left instanceof PropertyFetch || !$right instanceof PropertyFetch) { + if (! $left instanceof PropertyFetch || ! $right instanceof PropertyFetch) { return null; } $leftProperty = $left->name; $rightProperty = $right->name; - if (!$leftProperty instanceof Identifier || !$rightProperty instanceof Identifier) { + if (! $leftProperty instanceof Identifier || ! $rightProperty instanceof Identifier) { return null; } @@ -100,12 +99,14 @@ private function matchModelComparison(Equal|Identical $node): ?array if ($this->isForeignKeyToIdPattern($leftPropertyName, $rightPropertyName)) { // $model->foreign_key_id == $otherModel->id $relationshipName = $this->extractRelationshipName($leftPropertyName); + return [$left->var, $relationshipName, $right->var]; } if ($this->isForeignKeyToIdPattern($rightPropertyName, $leftPropertyName)) { // $otherModel->id == $model->foreign_key_id $relationshipName = $this->extractRelationshipName($rightPropertyName); + return [$right->var, $relationshipName, $left->var]; } @@ -124,30 +125,16 @@ private function extractRelationshipName(string $foreignKeyProperty): string private function couldBeModel(Expr $expr): bool { - $modelType = new ObjectType('Illuminate\Database\Eloquent\Model'); - + $objectType = new ObjectType('Illuminate\Database\Eloquent\Model'); + // For property fetch expressions like $user->team_id, check the variable part ($user) - if ($expr instanceof PropertyFetch) { - if ($this->isObjectType($expr->var, $modelType)) { - return true; - } - } - - // For variable expressions like $user, check the variable directly - if ($expr instanceof Variable) { - if ($this->isObjectType($expr, $modelType)) { - return true; - } + if ($expr instanceof PropertyFetch && $this->isObjectType($expr->var, $objectType)) { + return true; } - // Fallback: check for obvious non-model patterns - if ($expr instanceof Variable && is_string($expr->name)) { - $varName = $expr->name; - // Skip variables that are obviously not models - $nonModelNames = ['i', 'j', 'k', 'x', 'y', 'z', 'count', 'index', 'key', 'value', 'data', 'result', 'item', 'element']; - if (in_array($varName, $nonModelNames, true)) { - return false; - } + // For variable expressions like $user, check the variable directly + if ($expr instanceof Variable && $this->isObjectType($expr, $objectType)) { + return true; } // Allow by default for backwards compatibility - be permissive unless obviously not a model From 718f6491a1b00e4c92c37ede85fd943c38569e8e Mon Sep 17 00:00:00 2001 From: Liam Hammett Date: Mon, 6 Oct 2025 13:14:43 +0200 Subject: [PATCH 3/4] Code style fixes --- src/Rector/Expr/ModelComparisonToIsMethodRector.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Rector/Expr/ModelComparisonToIsMethodRector.php b/src/Rector/Expr/ModelComparisonToIsMethodRector.php index 96682da96..5b030a58f 100644 --- a/src/Rector/Expr/ModelComparisonToIsMethodRector.php +++ b/src/Rector/Expr/ModelComparisonToIsMethodRector.php @@ -127,17 +127,10 @@ private function couldBeModel(Expr $expr): bool { $objectType = new ObjectType('Illuminate\Database\Eloquent\Model'); - // For property fetch expressions like $user->team_id, check the variable part ($user) if ($expr instanceof PropertyFetch && $this->isObjectType($expr->var, $objectType)) { return true; } - // For variable expressions like $user, check the variable directly - if ($expr instanceof Variable && $this->isObjectType($expr, $objectType)) { - return true; - } - - // Allow by default for backwards compatibility - be permissive unless obviously not a model - return true; + return $expr instanceof Variable && $this->isObjectType($expr, $objectType); } } From 8d8a59dbaaab5cd6b230679c6e76f0d87751f0af Mon Sep 17 00:00:00 2001 From: Liam Hammett Date: Mon, 6 Oct 2025 13:37:34 +0200 Subject: [PATCH 4/4] Code style fixes --- .../Expr/ModelComparisonToIsMethodRector.php | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Rector/Expr/ModelComparisonToIsMethodRector.php b/src/Rector/Expr/ModelComparisonToIsMethodRector.php index 5b030a58f..c7e539a70 100644 --- a/src/Rector/Expr/ModelComparisonToIsMethodRector.php +++ b/src/Rector/Expr/ModelComparisonToIsMethodRector.php @@ -13,6 +13,7 @@ use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Identifier; +use PHPStan\Type\MixedType; use PHPStan\Type\ObjectType; use RectorLaravel\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; @@ -64,7 +65,6 @@ public function refactor(Node $node): ?Node [$leftVar, $relationshipName, $rightVar] = $result; - // Enhanced safety check: verify both variables could be models if (! $this->couldBeModel($leftVar) || ! $this->couldBeModel($rightVar)) { return null; } @@ -127,10 +127,24 @@ private function couldBeModel(Expr $expr): bool { $objectType = new ObjectType('Illuminate\Database\Eloquent\Model'); - if ($expr instanceof PropertyFetch && $this->isObjectType($expr->var, $objectType)) { - return true; + if ($expr instanceof PropertyFetch) { + $varType = $this->getType($expr->var); + if ($this->isObjectType($expr->var, $objectType)) { + return true; + } + + return $varType instanceof MixedType; + } + + if ($expr instanceof Variable) { + $varType = $this->getType($expr); + if ($this->isObjectType($expr, $objectType)) { + return true; + } + + return $varType instanceof MixedType; } - return $expr instanceof Variable && $this->isObjectType($expr, $objectType); + return false; } }