Skip to content

Commit 5b84aab

Browse files
sebdesigntaylorotwell
sebdesign
authored and
taylorotwell
committed
[7.x] Fix regressions on find* methods with Arrayable ids (#30312)
* 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.
1 parent 9e8cd8c commit 5b84aab

File tree

7 files changed

+136
-5
lines changed

7 files changed

+136
-5
lines changed

‎src/Illuminate/Database/Eloquent/Builder.php

+2
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,8 @@ public function findOrFail($id, $columns = ['*'])
360360
{
361361
$result = $this->find($id, $columns);
362362

363+
$id = $id instanceof Arrayable ? $id->toArray() : $id;
364+
363365
if (is_array($id)) {
364366
if (count($result) === count(array_unique($id))) {
365367
return $result;

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

+16-3
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Illuminate\Database\Eloquent\Relations;
44

5+
use Illuminate\Contracts\Support\Arrayable;
56
use Illuminate\Database\Eloquent\Builder;
67
use Illuminate\Database\Eloquent\Collection;
78
use Illuminate\Database\Eloquent\Model;
@@ -504,21 +505,31 @@ public function updateOrCreate(array $attributes, array $values = [], array $joi
504505
*/
505506
public function find($id, $columns = ['*'])
506507
{
507-
return is_array($id) ? $this->findMany($id, $columns) : $this->where(
508+
if (is_array($id) || $id instanceof Arrayable) {
509+
return $this->findMany($id, $columns);
510+
}
511+
512+
return $this->where(
508513
$this->getRelated()->getQualifiedKeyName(), '=', $id
509514
)->first($columns);
510515
}
511516

512517
/**
513518
* Find multiple related models by their primary keys.
514519
*
515-
* @param mixed $ids
520+
* @param \Illuminate\Contracts\Support\Arrayable|array $ids
516521
* @param array $columns
517522
* @return \Illuminate\Database\Eloquent\Collection
518523
*/
519524
public function findMany($ids, $columns = ['*'])
520525
{
521-
return empty($ids) ? $this->getRelated()->newCollection() : $this->whereIn(
526+
$ids = $ids instanceof Arrayable ? $ids->toArray() : $ids;
527+
528+
if (empty($ids)) {
529+
return $this->getRelated()->newCollection();
530+
}
531+
532+
return $this->whereIn(
522533
$this->getRelated()->getQualifiedKeyName(), $ids
523534
)->get($columns);
524535
}
@@ -536,6 +547,8 @@ public function findOrFail($id, $columns = ['*'])
536547
{
537548
$result = $this->find($id, $columns);
538549

550+
$id = $id instanceof Arrayable ? $id->toArray() : $id;
551+
539552
if (is_array($id)) {
540553
if (count($result) === count(array_unique($id))) {
541554
return $result;

‎src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php

+7-2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Illuminate\Database\Eloquent\Relations;
44

5+
use Illuminate\Contracts\Support\Arrayable;
56
use Illuminate\Database\Eloquent\Builder;
67
use Illuminate\Database\Eloquent\Collection;
78
use Illuminate\Database\Eloquent\Model;
@@ -285,7 +286,7 @@ public function firstOrFail($columns = ['*'])
285286
*/
286287
public function find($id, $columns = ['*'])
287288
{
288-
if (is_array($id)) {
289+
if (is_array($id) || $id instanceof Arrayable) {
289290
return $this->findMany($id, $columns);
290291
}
291292

@@ -297,12 +298,14 @@ public function find($id, $columns = ['*'])
297298
/**
298299
* Find multiple related models by their primary keys.
299300
*
300-
* @param mixed $ids
301+
* @param \Illuminate\Contracts\Support\Arrayable|array $ids
301302
* @param array $columns
302303
* @return \Illuminate\Database\Eloquent\Collection
303304
*/
304305
public function findMany($ids, $columns = ['*'])
305306
{
307+
$ids = $ids instanceof Arrayable ? $ids->toArray() : $ids;
308+
306309
if (empty($ids)) {
307310
return $this->getRelated()->newCollection();
308311
}
@@ -325,6 +328,8 @@ public function findOrFail($id, $columns = ['*'])
325328
{
326329
$result = $this->find($id, $columns);
327330

331+
$id = $id instanceof Arrayable ? $id->toArray() : $id;
332+
328333
if (is_array($id)) {
329334
if (count($result) === count(array_unique($id))) {
330335
return $result;

‎tests/Database/DatabaseEloquentBuilderTest.php

+11
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,17 @@ public function testFindOrFailMethodWithManyThrowsModelNotFoundException()
126126
$builder->findOrFail([1, 2], ['column']);
127127
}
128128

129+
public function testFindOrFailMethodWithManyUsingCollectionThrowsModelNotFoundException()
130+
{
131+
$this->expectException(ModelNotFoundException::class);
132+
133+
$builder = m::mock(Builder::class.'[get]', [$this->getMockQueryBuilder()]);
134+
$builder->setModel($this->getMockModel());
135+
$builder->getQuery()->shouldReceive('whereIn')->once()->with('foo_table.foo', [1, 2]);
136+
$builder->shouldReceive('get')->with(['column'])->andReturn(new Collection([1]));
137+
$builder->findOrFail(new Collection([1, 2]), ['column']);
138+
}
139+
129140
public function testFirstOrFailMethodThrowsModelNotFoundException()
130141
{
131142
$this->expectException(ModelNotFoundException::class);

‎tests/Database/DatabaseEloquentHasManyThroughIntegrationTest.php

+59
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use Illuminate\Database\Eloquent\Model as Eloquent;
77
use Illuminate\Database\Eloquent\ModelNotFoundException;
88
use Illuminate\Database\Eloquent\SoftDeletes;
9+
use Illuminate\Support\Collection;
910
use Illuminate\Support\LazyCollection;
1011
use PHPUnit\Framework\TestCase;
1112

@@ -120,6 +121,40 @@ public function testWhereHasOnARelationWithCustomIntermediateAndLocalKey()
120121
$this->assertCount(1, $country);
121122
}
122123

124+
public function testFindMethod()
125+
{
126+
HasManyThroughTestCountry::create(['id' => 1, 'name' => 'United States of America', 'shortname' => 'us'])
127+
->users()->create(['id' => 1, 'email' => '[email protected]', 'country_short' => 'us'])
128+
->posts()->createMany([
129+
['id' => 1, 'title' => 'A title', 'body' => 'A body', 'email' => '[email protected]'],
130+
['id' => 2, 'title' => 'Another title', 'body' => 'Another body', 'email' => '[email protected]'],
131+
]);
132+
133+
$country = HasManyThroughTestCountry::first();
134+
$post = $country->posts()->find(1);
135+
136+
$this->assertNotNull($post);
137+
$this->assertSame('A title', $post->title);
138+
139+
$this->assertCount(2, $country->posts()->find([1, 2]));
140+
$this->assertCount(2, $country->posts()->find(new Collection([1, 2])));
141+
}
142+
143+
public function testFindManyMethod()
144+
{
145+
HasManyThroughTestCountry::create(['id' => 1, 'name' => 'United States of America', 'shortname' => 'us'])
146+
->users()->create(['id' => 1, 'email' => '[email protected]', 'country_short' => 'us'])
147+
->posts()->createMany([
148+
['id' => 1, 'title' => 'A title', 'body' => 'A body', 'email' => '[email protected]'],
149+
['id' => 2, 'title' => 'Another title', 'body' => 'Another body', 'email' => '[email protected]'],
150+
]);
151+
152+
$country = HasManyThroughTestCountry::first();
153+
154+
$this->assertCount(2, $country->posts()->findMany([1, 2]));
155+
$this->assertCount(2, $country->posts()->findMany(new Collection([1, 2])));
156+
}
157+
123158
public function testFirstOrFailThrowsAnException()
124159
{
125160
$this->expectException(ModelNotFoundException::class);
@@ -142,6 +177,30 @@ public function testFindOrFailThrowsAnException()
142177
HasManyThroughTestCountry::first()->posts()->findOrFail(1);
143178
}
144179

180+
public function testFindOrFailWithManyThrowsAnException()
181+
{
182+
$this->expectException(ModelNotFoundException::class);
183+
$this->expectExceptionMessage('No query results for model [Illuminate\Tests\Database\HasManyThroughTestPost] 1, 2');
184+
185+
HasManyThroughTestCountry::create(['id' => 1, 'name' => 'United States of America', 'shortname' => 'us'])
186+
->users()->create(['id' => 1, 'email' => '[email protected]', 'country_short' => 'us'])
187+
->posts()->create(['id' => 1, 'title' => 'A title', 'body' => 'A body', 'email' => '[email protected]']);
188+
189+
HasManyThroughTestCountry::first()->posts()->findOrFail([1, 2]);
190+
}
191+
192+
public function testFindOrFailWithManyUsingCollectionThrowsAnException()
193+
{
194+
$this->expectException(ModelNotFoundException::class);
195+
$this->expectExceptionMessage('No query results for model [Illuminate\Tests\Database\HasManyThroughTestPost] 1, 2');
196+
197+
HasManyThroughTestCountry::create(['id' => 1, 'name' => 'United States of America', 'shortname' => 'us'])
198+
->users()->create(['id' => 1, 'email' => '[email protected]', 'country_short' => 'us'])
199+
->posts()->create(['id' => 1, 'title' => 'A title', 'body' => 'A body', 'email' => '[email protected]']);
200+
201+
HasManyThroughTestCountry::first()->posts()->findOrFail(new Collection([1, 2]));
202+
}
203+
145204
public function testFirstRetrievesFirstRecord()
146205
{
147206
$this->seedData();

‎tests/Database/DatabaseEloquentIntegrationTest.php

+9
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,15 @@ public function testFindOrFailWithMultipleIdsThrowsModelNotFoundException()
475475
EloquentTestUser::findOrFail([1, 2]);
476476
}
477477

478+
public function testFindOrFailWithMultipleIdsUsingCollectionThrowsModelNotFoundException()
479+
{
480+
$this->expectException(ModelNotFoundException::class);
481+
$this->expectExceptionMessage('No query results for model [Illuminate\Tests\Database\EloquentTestUser] 1, 2');
482+
483+
EloquentTestUser::create(['id' => 1, 'email' => '[email protected]']);
484+
EloquentTestUser::findOrFail(new Collection([1, 2]));
485+
}
486+
478487
public function testOneToOneRelationship()
479488
{
480489
$user = EloquentTestUser::create(['email' => '[email protected]']);

‎tests/Integration/Database/EloquentBelongsToManyTest.php

+32
Original file line numberDiff line numberDiff line change
@@ -347,12 +347,16 @@ public function testFindMethod()
347347
$post->tags()->attach(Tag::all());
348348

349349
$this->assertEquals($tag2->name, $post->tags()->find($tag2->id)->name);
350+
$this->assertCount(0, $post->tags()->findMany([]));
350351
$this->assertCount(2, $post->tags()->findMany([$tag->id, $tag2->id]));
352+
$this->assertCount(0, $post->tags()->findMany(new Collection()));
353+
$this->assertCount(2, $post->tags()->findMany(new Collection([$tag->id, $tag2->id])));
351354
}
352355

353356
public function testFindOrFailMethod()
354357
{
355358
$this->expectException(ModelNotFoundException::class);
359+
$this->expectExceptionMessage('No query results for model [Illuminate\Tests\Integration\Database\EloquentBelongsToManyTest\Tag] 10');
356360

357361
$post = Post::create(['title' => Str::random()]);
358362

@@ -363,6 +367,34 @@ public function testFindOrFailMethod()
363367
$post->tags()->findOrFail(10);
364368
}
365369

370+
public function testFindOrFailMethodWithMany()
371+
{
372+
$this->expectException(ModelNotFoundException::class);
373+
$this->expectExceptionMessage('No query results for model [Illuminate\Tests\Integration\Database\EloquentBelongsToManyTest\Tag] 10, 11');
374+
375+
$post = Post::create(['title' => Str::random()]);
376+
377+
Tag::create(['name' => Str::random()]);
378+
379+
$post->tags()->attach(Tag::all());
380+
381+
$post->tags()->findOrFail([10, 11]);
382+
}
383+
384+
public function testFindOrFailMethodWithManyUsingCollection()
385+
{
386+
$this->expectException(ModelNotFoundException::class);
387+
$this->expectExceptionMessage('No query results for model [Illuminate\Tests\Integration\Database\EloquentBelongsToManyTest\Tag] 10, 11');
388+
389+
$post = Post::create(['title' => Str::random()]);
390+
391+
Tag::create(['name' => Str::random()]);
392+
393+
$post->tags()->attach(Tag::all());
394+
395+
$post->tags()->findOrFail(new Collection([10, 11]));
396+
}
397+
366398
public function testFindOrNewMethod()
367399
{
368400
$post = Post::create(['title' => Str::random()]);

0 commit comments

Comments
 (0)