Convert model ID comparisons to use the is() method #398
Conversation
imliam
commented
Oct 4, 2025
imliam
commented
| if (! $node instanceof Equal && ! $node instanceof Identical) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This might be a redundant check, since we're always passing Equal or Identical.
There was a problem hiding this comment.
It is. The docblock removes any PHPStan ambiguity.
| 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); | ||
| } |
There was a problem hiding this comment.
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 getKeyName() method or the $primaryKey property.
Also, it fails when relationship names are complex (nationalTeam() with national_team_id key), or don't exist (external_team_id, but no externalTeam() relationship).
The only way this could work well would be to check the relationships.
There was a problem hiding this comment.
I agree. In theory we can build better analysers for this, to look at the model, then the relationship methods on the model, then to determine with default or custom keys are used. It's possible but complicated. Larastan project might have something like this already to base the analyser off of.
| /** | ||
| * @param Equal|Identical $node | ||
| */ | ||
| public function refactor(Node $node): ?Node |
There was a problem hiding this comment.
| public function refactor(Node $node): ?Node | |
| public function refactor(Node $node): Equal|Identical|null |
| class User | ||
| { | ||
| public $id; | ||
| } | ||
|
|
||
| class Post | ||
| { | ||
| public $author_id; | ||
| public $editor_id; | ||
|
|
||
| public function author() | ||
| { | ||
| return $this->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); | ||
| } | ||
| } |
There was a problem hiding this comment.
We don't typically define all this in the fixtures. Instead in the test folder we add a Source folder and inside that define classes we can reference in the fixtures. Leads to less to read and human error when updating fixtures.
3 participants
