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

[8.x] Add whereBelongsTo() Eloquent builder method #38927

Merged
merged 14 commits into from
Sep 28, 2021
Merged

[8.x] Add whereBelongsTo() Eloquent builder method #38927

merged 14 commits into from
Sep 28, 2021

Conversation

danharrin
Copy link
Contributor

@danharrin danharrin commented Sep 23, 2021

Hey 👋

When retrieving belongsTo related records, you can usually use the inverse (hasMany) of the relationship, like so:

$author->posts()

However, sometimes this is not possible, and you must filter an existing query by its parent record:

$query->where('author_id', $author->id)

This works, but explicitly depends on the name of foreign key (author_id), and the name of the owner key (id) in most cases. This is a maintenance burden, as these key names can be changed inside the relationship method and will subsequently become outdated across your entire app.

This PR introduces a whereBelongsTo() method to the Eloquent builder. You may use it like so:

$query->whereBelongsTo($author)

It will automatically retrieve the foreign key name from the relationship (author), and the correct owner key from the related model ($author).

By default, the name of the relationship is guessed based on the class of the related model. Sometimes, this is not appropriate. If the $author is an instance of App\Models\User, whereBelongsTo() will search for a user() relationship that does not exist. In this case, you may manually specify the relationship name:

$query->whereBelongsTo($author, 'author')

@ankurk91
Copy link
Contributor

There is already a method for this purpose whereRelation()
#38499

The whereBelongsTo could be a breaking change for many, because laravel auto detect the column name from method name.
For example:

# magic method
->whereCountry("USA")

# will become
->where("country", "USA")

so someone might be using it like $house->whereBelongsTo("realtor") in their code-base which is being translating to where("belongs_to", "realtor")

If this PR gets merged, people will be sending similar PRs for all of other relations.

I would suggest not to introduce more alias methods in query builder, as this increase the learning curve.

@danharrin
Copy link
Contributor Author

danharrin commented Sep 23, 2021

Thanks for the feedback, but I disagree on all points.

There is already a method for this purpose whereRelation()

Please read through my description. I don't think whereRelation() covers the use case of this feature.

The whereBelongsTo could be a breaking change for many, because laravel auto detect the column name from method name.

The possibility of someone using belongs_to AND whereBelongsTo is incredibly small. If this is a breaking change, #38499 should not have been merged in a minor release either - arguably more of a common breaking change.

If this PR gets merged, people will be sending similar PRs for all of other relations.

Not really, most of the relationship types are not suitable for this sort of method. That only applies to relationship types whereby the foreign key is on the related model. So - only morphTo.

I would suggest not to introduce more alias methods in query builder, as this increase the learning curve.

This is not an increase to the "learning curve". That only really happens when extra features are introduced that you must use, or changes that make existing features too complex. This is an optional helper method that improves code clarity and maintainability in many places throughout an app.

@bert-w
Copy link
Contributor

bert-w commented Sep 23, 2021

Can you make this work with $query->whereBelongsTo(Author::class)? I'm not sure how $query->whereBelongsTo($author) is useful if you have access to the $author object already.

@danharrin
Copy link
Contributor Author

Can you make this work with $query->whereBelongsTo(Author::class)? I'm not sure how $query->whereBelongsTo($author) is useful if you have access to the $author object already.

Please read my description. I don't know how passing the class name here is useful.

@bert-w
Copy link
Contributor

bert-w commented Sep 24, 2021

Ah I think I see it now. So in which situations are you bound to using $query->where('author_id', $author->id)->moreStuffHere() vs $author->posts()->moreStuffHere()? You say "sometimes this is not possible" in the original post but I dont think I have encountered those situations myself (or I have silently worked around them).

@danharrin
Copy link
Contributor Author

danharrin commented Sep 24, 2021

Ah I think I see it now. So in which situations are you bound to using $query->where('author_id', $author->id)->moreStuffHere() vs $author->posts()->moreStuffHere()? You say "sometimes this is not possible" in the original post but I dont think I have encountered those situations myself (or I have silently worked around them).

For example, this is a pretty common case when you're inside a multitenant application. I'll give you a couple examples:

Listing an author's posts inside a particular category:

$author->posts()->whereBelongsTo($category)->get()

Filtering a list of songs by album and artist:

Song::query()->whereBelongsTo($album)->whereBelongsTo($artist)->get()

@taylorotwell
Copy link
Member

Kinda feels like some of these checks should throw exceptions instead of just returning $this. Like if the given class isn't part of a relationship, etc.

@danharrin
Copy link
Contributor Author

Kinda feels like some of these checks should throw exceptions instead of just returning $this. Like if the given class isn't part of a relationship, etc.

All done.

@bert-w
Copy link
Contributor

bert-w commented Sep 24, 2021

I think you should use RelationNotFoundException. The existing source code also seems to handle this differently by just calling the method and see if it throws a BadMethodCallException, see:

try {
return $this->getModel()->newInstance()->$name();
} catch (BadMethodCallException $e) {
throw RelationNotFoundException::make($this->getModel(), $name);
}

@danharrin
Copy link
Contributor Author

I think you should use RelationNotFoundException. The existing source code also seems to handle this differently by just calling the method and see if it throws a BadMethodCallException, see:

try {
return $this->getModel()->newInstance()->$name();
} catch (BadMethodCallException $e) {
throw RelationNotFoundException::make($this->getModel(), $name);
}

Done that too. I've also added an extra property to RelationNotFoundException to report if a relation is not found with the correct type.

@taylorotwell taylorotwell merged commit 43c8bff into laravel:8.x Sep 28, 2021
@danharrin danharrin deleted the feature/where-belongs-to branch September 28, 2021 19:31
@mitoop
Copy link
Contributor

mitoop commented Oct 7, 2021

@danharrin whereBelongsTo and orWhereBelongsTo methods phpdoc can adjust a little

victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* Add failing test for whereBelongsTo

* Implement whereBelongsTo

* mock in tests

* Add test for relationship guess

* Add exceptions

* Remove early returns

* Update Builder.php

* Update Builder.php

* Throw RelationNotFoundException

* Refactor to catch BadMethodCallException

* Remove rogue get_class

* move method to trait

* qualify foreign key

* formatting

Co-authored-by: Taylor Otwell <taylorotwell@gmail.com>
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