Skip to content

[7.x] Add extendable relations for models#33025

Merged
taylorotwell merged 3 commits intolaravel:7.xfrom
iamgergo:7.x
Jun 3, 2020
Merged

[7.x] Add extendable relations for models#33025
taylorotwell merged 3 commits intolaravel:7.xfrom
iamgergo:7.x

Conversation

@iamgergo
Copy link
Contributor

@iamgergo iamgergo commented May 30, 2020

This PR introduces the resolveRelationUsing method, which allows defining relationships between models from "outside" of the classes. For example:

Order::resolveRelationUsing('customer', function ($model) {
    return $model->belongsTo(Customer::class, 'customer_id');
});

$order->customer() // relation instance

$order->customer // related model instance / collection

$order->customer()->associate(...)

Order::whereHas('customer', function ($query) {
    //
});

Order::with('customer')->where(...);

// All should work as we would define the relation in the model class

This extension could be very handy when building multiple packages in the same ecosystem that may extend each other's functionality. Of course often extending models, using traits or writing query builder macros is enough, but all of them have limitations at some point.


Keys should be passed explicitly when defining the relationship, to avoid automatically generated keys for a closure: {closure}_id.

By default, this should not be a breaking change, at least I could not find any. However, any defined resolvers will be triggered before the call is forwarded to the query builder. But by default, this should not affect any existing code, since the resolvers array is empty.

Also, this would not affect any property or method that the class already has, they should have priority over the relation definitions.


I found a related PR #22507, however, this approach tries to achieve to provide the same behavior as it would be a "native" relation definition. Also, the implementation of the two PRs are quite different.

@imanghafoori1
Copy link
Contributor

https://github.com/imanghafoori1/eloquent-relativity

My package did the same job, but it was faced with a lot of hate and anger in laravel/ideas, sadly.

@iamgergo
Copy link
Contributor Author

@imanghafoori1 Nice, I did not know about that package.

I'm sorry to hear. I really don't understand why. I find this very useful in some cases.

@imanghafoori1
Copy link
Contributor

imanghafoori1 commented Jun 11, 2020

@iamgergo You work is absolutely welcome, I had a long discussion about the idea long before about this.
laravel/ideas#1473
But I was disappointed because of the negative feedback.
Anyway, I am happy to see that you dared to make it go to the core, it was a real need.

@deleugpn
Copy link
Contributor

@iamgergo Thanks so much for this. Such a simple implementation with so much potential.

@meletisf
Copy link

meletisf commented Aug 4, 2020

@imanghafoori1 this makes perfect sense. I'm using an architecture in which the parent cannot know about the models of the sub-modules therefore i could not use traits.

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.

5 participants