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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 3, 2020
Merged

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

merged 3 commits into from
Jun 3, 2020

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