Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Add is() method to 1-1 relations for model comparison #34693

Merged
merged 1 commit into from Oct 6, 2020

Conversation

sebdesign
Copy link
Contributor

@sebdesign sebdesign commented Oct 5, 2020

This PR adds replicates the is() and isNot()methods of the Illuminate\Database\Eloquent\Model to the 1-1 Eloquent relations: BelongsTo, HasOne, MorphTo and MorphOne.

We can now do model comparisons between related models, without extra database calls! This is especially useful in gates/policies when we need to know if a model is owned or owns another model.

Example on a PostPolicy with a simple BelongsTo relationship:

public function publish(User $user, Post $post)
{
    // Before: foreign key is leaking from the post model
    return $post->author_id === $user->id;

    // Before: performs extra query to fetch the user model from the author relation
    return $post->author->is($user);

    // After 馃殌
    return $post->author()->is($user);
}

Example with a polymorphic MorphOne relationship:

// Before
return $attachment->attachable_id === $post->getKey()
    && $attachment->attachable_type === $post->getMorphClass();

// Before
return $post->attachment->is($attachment);

// After 馃殌
return $post->attachment()->is($attachment);

The implementation is similar to the Model, it compares the keys, the tables, and the connections of two models. But instead of comparing the primary keys using Model::getKey(), it compares the parent key of the parent model of the relationship with the foreign key of the given model. Both key names are known to the relation instance, so the comparison is straightforward.

The only thing that is not really accurate is the type of the keys. A model usually has an integer or a string primary key, but the foreign keys are not cast to the related model's primary key type, or sometimes the relationships do not include any primary key, but are constrained by other keys. For this reason, if one of the keys is an integer, we are casting both keys to integers before comparing them, otherwise we are comparing them as they are.

For convenience, the functionality is defined in a ComparesModels trait, so it can be used on custom relationships.
This trait needs two methods to be implemented:

  1. the getParentKey() which already existed on HasOne/MorphOne relations
  2. the getRelatedKeyFrom($model), which is new, but is quite useful because it can be reused in many places in the existing relations codebase, instead of calling $model->{$this->ownerKey}, $model->{$foreign}, $parent->{$this->localKey}, etc. in so many different ways. I've refrained myself from refactoring these calls, but I can do that in a next PR.

This PR was inspired by this discussion on Reddit: https://www.reddit.com/r/laravel/comments/iylnnl/this_is_how_we_write_our_policies_now.

Both unit and integration tests are in place.

PS: I'm also pinging @staudenmeir since he has a deep understanding of eloquent relations.

@driesvints driesvints changed the title Add is() method to 1-1 relations for model comparison [8.x] Add is() method to 1-1 relations for model comparison Oct 6, 2020
@taylorotwell taylorotwell merged commit 963f698 into laravel:8.x Oct 6, 2020
@taylorotwell
Copy link
Member

Thanks

@sebdesign
Copy link
Contributor Author

Thank you @taylorotwell! Your ideas are an inspiration for features like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants