-
Notifications
You must be signed in to change notification settings - Fork 88
Convert model ID comparisons to use the is() method #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace RectorLaravel\Rector\Expr; | ||
|
|
||
| use PhpParser\Node; | ||
| use PhpParser\Node\Arg; | ||
| use PhpParser\Node\Expr; | ||
| use PhpParser\Node\Expr\BinaryOp\Equal; | ||
| use PhpParser\Node\Expr\BinaryOp\Identical; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| 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; | ||
| use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; | ||
|
|
||
| /** | ||
| * @see \RectorLaravel\Tests\Rector\Expr\ModelComparisonToIsMethodRector\ModelComparisonToIsMethodRectorTest | ||
| */ | ||
| final class ModelComparisonToIsMethodRector extends AbstractRector | ||
| { | ||
| public function getRuleDefinition(): RuleDefinition | ||
| { | ||
| return new RuleDefinition( | ||
| 'Convert model ID comparisons to use the is() method', | ||
| [new CodeSample( | ||
| <<<'CODE_SAMPLE' | ||
| $team->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<class-string<Node>> | ||
| */ | ||
| 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; | ||
|
|
||
| if (! $this->couldBeModel($leftVar) || ! $this->couldBeModel($rightVar)) { | ||
| return null; | ||
| } | ||
|
|
||
| $methodCall = new MethodCall($leftVar, new Identifier($relationshipName)); | ||
|
|
||
| return new MethodCall($methodCall, new Identifier('is'), [new Arg($rightVar)]); | ||
| } | ||
|
|
||
| /** | ||
| * @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); | ||
| } | ||
|
Comment on lines
+116
to
+124
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid this part here won't work well with a lot of cases. It assumes that the key is always 'id' or follows the standard '_id'. I'm not sure if we can find the key name from the model's Also, it fails when relationship names are complex ( The only way this could work well would be to check the relationships. |
||
|
|
||
| private function couldBeModel(Expr $expr): bool | ||
| { | ||
| $objectType = new ObjectType('Illuminate\Database\Eloquent\Model'); | ||
|
|
||
| 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 false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| <?php | ||
|
|
||
| namespace RectorLaravel\Tests\Rector\Expr\ModelComparisonToIsMethodRector\Fixture; | ||
|
|
||
| use Illuminate\Database\Eloquent\Model; | ||
|
|
||
| class Comment extends Model | ||
| { | ||
| public $post_id; | ||
| public $author_id; | ||
|
|
||
| public function post() | ||
| { | ||
| return $this->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; | ||
|
|
||
| ?> | ||
| ----- | ||
| <?php | ||
|
|
||
| namespace RectorLaravel\Tests\Rector\Expr\ModelComparisonToIsMethodRector\Fixture; | ||
|
|
||
| use Illuminate\Database\Eloquent\Model; | ||
|
|
||
| class Comment extends Model | ||
| { | ||
| public $post_id; | ||
| public $author_id; | ||
|
|
||
| public function post() | ||
| { | ||
| return $this->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); | ||
|
|
||
| ?> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| <?php | ||
|
|
||
| namespace RectorLaravel\Tests\Rector\Expr\ModelComparisonToIsMethodRector\Fixture; | ||
|
|
||
| use Illuminate\Database\Eloquent\Model; | ||
|
|
||
| class User extends Model | ||
| { | ||
| public $id; | ||
| } | ||
|
|
||
| class Team extends Model | ||
| { | ||
| public $user_id; | ||
|
|
||
| public function user() | ||
| { | ||
| return $this->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; | ||
|
|
||
| ?> | ||
| ----- | ||
| <?php | ||
|
|
||
| namespace RectorLaravel\Tests\Rector\Expr\ModelComparisonToIsMethodRector\Fixture; | ||
|
|
||
| use Illuminate\Database\Eloquent\Model; | ||
|
|
||
| class User extends Model | ||
| { | ||
| public $id; | ||
| } | ||
|
|
||
| class Team extends Model | ||
| { | ||
| public $user_id; | ||
|
|
||
| public function user() | ||
| { | ||
| return $this->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); | ||
|
|
||
| ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a redundant check, since we're always passing Equal or Identical.