[5.1] Add missing *orFail methods in relationships #9143

lukasgeiter · 2015-06-08T20:00:33Z

Fixes #9138

This adds findOrFail to BelongsToMany and findOrFail/firstOrFail to HasManyThrough

taylorotwell · 2015-06-08T20:46:08Z

I'll be honest I have a little bit of a hard time understanding the value of these methods. Why not just use the related model directly?

lukasgeiter · 2015-06-08T20:49:55Z

Hmm that's true. However why does find() exist then?

taylorotwell · 2015-06-08T21:44:46Z

Not sure. To me it makes more sense to just go straight to the model you want. No point in going through the relationship.

lukasgeiter · 2015-06-08T21:48:08Z

I agree with you. Should we deprecate the two find methods then?

stevebauman · 2015-07-06T17:52:25Z

@taylorotwell I disagree. Going through the relationship methods help a ton when creating / finding records for belongsToMany() relations and others since query fields are auto-filled and filter the results that are attached to that record.

For an example, I can make sure that a Book has an Author of a specific ID, by using:

$book = Book::find($id);

$author = $book->authors()->find($authorId);

If I was using the related model directly, I'd need to query through the pivot table manually, or by using a $book->whereHas('authors') or something more complicated to just find the a specific author of the book (and in this case I'm using the relation anyway).

Going through a relationship is also great because you have a single point where your foreign key is declared in your relationship, and if I ever need to change it, I literally just change the string in the belongsToMany() relation.

Querying relations is advertised in the documentation for versions 4.2, 5.0, 5.1, and Master:

http://laravel.com/docs/master/eloquent-relationships#querying-relations

I really think these methods should be added to keep consistency with the query builder API. I'd really like to use $book->authors()->findOrFail($id); so I can continue catching ModelNotFoundException's and return the appropriate response.

Please let me know your thoughts, thanks!

jarektkaczyk · 2015-07-18T12:13:57Z

@lukasgeiter @taylorotwell This one should be merged.

Lack of find* in relations leads to inconsistency - along with #9647.

It's helpful to have the relation constraint applied automatically when calling $model->relation()->findOrFail() and, for BelongsToMany, it is required, since otherwise there's pivot data missing from the result.

taylorotwell · 2015-07-18T12:44:39Z

I still don't understand why even go through the relationship at all if you
already know the primary key of the model you are trying to find. Going
through the relationship is just a waste of resources at that point.

On Saturday, July 18, 2015, Jarek Tkaczyk [email protected] wrote:

@lukasgeiter https://github.com/lukasgeiter @taylorotwell
https://github.com/taylorotwell This one should be merged.

Lack of find* in relations leads to inconsistency - along with #9647
#9647.

It's helpful to have the relation constraint applied automatically when
calling $model->relation()->findOrFail() and, for BelongsToMany, it is
required, since otherwise there's pivot data missing from the result.

—
Reply to this email directly or view it on GitHub
#9143 (comment).

jarektkaczyk · 2015-07-18T12:49:36Z

@taylorotwell Because you may want to check whether, say, a user owns a resource:

$loggedInUser->resources()->findOrFail($id)

rather than

Resource->whereHas('user', function ($q) use ($loggedInUser) {
    $q->where('id', $loggedInUser->id);
})

stevebauman · 2015-07-18T18:08:11Z

Also, without going through the relationship, we'd need to retrieve extra pivot data manually, would we not?

bryannielsen · 2015-07-20T15:04:23Z

I've been using this the same way @jarektkaczyk mentioned, if there's a more appropriate alternative way to do this I'm happy to change my ways, but this seems to be a pretty quick and convenient way to ensure ownership on related models.

stevebauman · 2015-08-06T19:31:50Z

Any update on this?

GrahamCampbell · 2015-08-06T20:02:45Z

This needs rebasing.

lukasgeiter · 2015-08-06T20:29:36Z

This needs rebasing.

Done.

GrahamCampbell · 2015-08-06T20:30:35Z

src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php

+        $result = $this->find($id, $columns);
+
+        if (is_array($id)) {
+            if (count($result) == count(array_unique($id))) {


=== please

unless our other code uses == for some strange reason

Well the Eloquent Builder uses ==

taylorotwell · 2015-08-09T01:44:41Z

OK I'm confused again. I already use findOrFail on a belongsToMany relation many times in my last production project without this pull request. What am I missing?

[5.1] Add missing *orFail methods in relationships

taylorotwell · 2015-08-09T01:45:56Z

Nevermind, read original bug description.

…ptions The regression in laravel#7048 and laravel#19019 is also observed in laravel#9143, because the `find`, `findMany` and `findOrFail` methods are copied from the Eloquent Builder to the `BelongsToMany` and `HasManyThrough` relations. For this reason, we need to convert the passed ids to an array before executing the queries.

The regression in laravel#7048 and laravel#19019 is also observed in laravel#9143, because the `find`, `findMany` and `findOrFail` methods are copied from the Eloquent Builder to the `BelongsToMany` and `HasManyThrough` relations. For this reason, we need to convert the passed ids to an array before executing the queries.

…regression in laravel#7048 and laravel#19019 is also observed in laravel#9143, because the `find`, `findMany` and `findOrFail` methods are copied from the Eloquent Builder to the `BelongsToMany` and `HasManyThrough` relations.For this reason, we need to convert the passed ids to an array before executing the queries.

* Ensure Builder::findOrFail with Arrayable throws ModelNotFoundException I found a regression in #7048 and #19019, where the Eloquent Builder will not throw a `ModelNotFoundException` when an `Arrayable` is passed to `findOrFail`. In this #7048, we are checking if the `$ids` is an array and we count the results against the number of ids. But since #19019, the `find` method also accepts an `Arrayable` for the ids. So if an `Arrayable` is passed, the check is skipped and the method returns the results. To fix this, we are first checking if the `$ids` is an `Arrayable` and we convert the ids to an array before checking the results. * Ensure find* methods on relationships are accepting Arrayable idsThe regression in #7048 and #19019 is also observed in #9143, because the `find`, `findMany` and `findOrFail` methods are copied from the Eloquent Builder to the `BelongsToMany` and `HasManyThrough` relations.For this reason, we need to convert the passed ids to an array before executing the queries.

lukasgeiter mentioned this pull request Jun 8, 2015

[5.1] Using findOrFail on belongsToMany relations doesn't return pivot #9138

Closed

taylorotwell closed this Jun 8, 2015

taylorotwell reopened this Aug 6, 2015

Add missing *orFail methods in relationships

1febc31

GrahamCampbell added proposal labels Aug 6, 2015

GrahamCampbell reviewed Aug 6, 2015
View reviewed changes

taylorotwell added a commit that referenced this pull request Aug 9, 2015

Merge pull request #9143 from lukasgeiter/find-or-fail

f4f3c0d

[5.1] Add missing *orFail methods in relationships

taylorotwell merged commit f4f3c0d into laravel:5.1 Aug 9, 2015

lukasgeiter deleted the find-or-fail branch August 9, 2015 13:32

sebdesign mentioned this pull request Oct 16, 2019

[7.x] Fix regressions on find* methods with Arrayable ids #30312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.1] Add missing *orFail methods in relationships #9143

[5.1] Add missing *orFail methods in relationships #9143

lukasgeiter commented Jun 8, 2015

taylorotwell commented Jun 8, 2015

lukasgeiter commented Jun 8, 2015

taylorotwell commented Jun 8, 2015

lukasgeiter commented Jun 8, 2015

stevebauman commented Jul 6, 2015

jarektkaczyk commented Jul 18, 2015

taylorotwell commented Jul 18, 2015

jarektkaczyk commented Jul 18, 2015

stevebauman commented Jul 18, 2015

bryannielsen commented Jul 20, 2015

stevebauman commented Aug 6, 2015

GrahamCampbell commented Aug 6, 2015

lukasgeiter commented Aug 6, 2015

GrahamCampbell Aug 6, 2015

GrahamCampbell Aug 6, 2015

lukasgeiter Aug 6, 2015

taylorotwell commented Aug 9, 2015

taylorotwell commented Aug 9, 2015

[5.1] Add missing *orFail methods in relationships #9143

[5.1] Add missing *orFail methods in relationships #9143

Conversation

lukasgeiter commented Jun 8, 2015

taylorotwell commented Jun 8, 2015

lukasgeiter commented Jun 8, 2015

taylorotwell commented Jun 8, 2015

lukasgeiter commented Jun 8, 2015

stevebauman commented Jul 6, 2015

jarektkaczyk commented Jul 18, 2015

taylorotwell commented Jul 18, 2015

jarektkaczyk commented Jul 18, 2015

stevebauman commented Jul 18, 2015

bryannielsen commented Jul 20, 2015

stevebauman commented Aug 6, 2015

GrahamCampbell commented Aug 6, 2015

lukasgeiter commented Aug 6, 2015

GrahamCampbell Aug 6, 2015

Choose a reason for hiding this comment

GrahamCampbell Aug 6, 2015

Choose a reason for hiding this comment

lukasgeiter Aug 6, 2015

Choose a reason for hiding this comment

taylorotwell commented Aug 9, 2015

taylorotwell commented Aug 9, 2015