Skip to content

[12.x] Improve circular relation check in Automatic Relation Loading #55542

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Illuminate/Database/Eloquent/Collection.php
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ public function withRelationshipAutoloading()

foreach ($this as $model) {
if (! $model->hasRelationAutoloadCallback()) {
$model->autoloadRelationsUsing($callback);
$model->autoloadRelationsUsing($callback, $this);
}
}

Expand Down
25 changes: 17 additions & 8 deletions src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ trait HasRelationships
*/
protected $relationAutoloadCallback = null;

/**
* The relationship autoloader callback context.
*
* @var mixed
*/
protected $relationAutoloadContext = null;

/**
* The many to many relationship methods.
*
Expand Down Expand Up @@ -118,10 +125,16 @@ public function hasRelationAutoloadCallback()
*/
public function autoloadRelationsUsing(Closure $callback, $context = null)
{
// Prevent circular relation autoloading...
if ($context && $this->relationAutoloadContext === $context) {
return $this;
}

$this->relationAutoloadCallback = $callback;
$this->relationAutoloadContext = $context;

foreach ($this->relations as $key => $value) {
$this->propagateRelationAutoloadCallbackToRelation($key, $value, $context);
$this->propagateRelationAutoloadCallbackToRelation($key, $value);
}

return $this;
Expand Down Expand Up @@ -163,10 +176,9 @@ protected function invokeRelationAutoloadCallbackFor($key, $tuples)
*
* @param string $key
* @param mixed $models
* @param mixed $context
* @return void
*/
protected function propagateRelationAutoloadCallbackToRelation($key, $models, $context = null)
protected function propagateRelationAutoloadCallbackToRelation($key, $models)
{
if (! $this->hasRelationAutoloadCallback() || ! $models) {
return;
Expand All @@ -183,10 +195,7 @@ protected function propagateRelationAutoloadCallbackToRelation($key, $models, $c
$callback = fn (array $tuples) => $this->invokeRelationAutoloadCallbackFor($key, $tuples);

foreach ($models as $model) {
// Check if relation autoload contexts are different to avoid circular relation autoload...
if ((is_null($context) || $context !== $model) && is_object($model) && method_exists($model, 'autoloadRelationsUsing')) {
$model->autoloadRelationsUsing($callback, $context);
}
$model->autoloadRelationsUsing($callback, $this->relationAutoloadContext);
}
}

Expand Down Expand Up @@ -1111,7 +1120,7 @@ public function setRelation($relation, $value)
{
$this->relations[$relation] = $value;

$this->propagateRelationAutoloadCallbackToRelation($relation, $value, $this);
$this->propagateRelationAutoloadCallbackToRelation($relation, $value);

return $this;
}
Expand Down
55 changes: 55 additions & 0 deletions tests/Integration/Database/EloquentModelRelationAutoloadTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ public function testRelationAutoloadForCollection()
$this->assertCount(2, DB::getQueryLog());
$this->assertCount(3, $likes);
$this->assertTrue($posts[0]->comments[0]->relationLoaded('likes'));

DB::disableQueryLog();
}

public function testRelationAutoloadForSingleModel()
Expand All @@ -84,6 +86,8 @@ public function testRelationAutoloadForSingleModel()
$this->assertCount(2, DB::getQueryLog());
$this->assertCount(2, $likes);
$this->assertTrue($post->comments[0]->relationLoaded('likes'));

DB::disableQueryLog();
}

public function testRelationAutoloadWithSerialization()
Expand All @@ -109,6 +113,50 @@ public function testRelationAutoloadWithSerialization()
$this->assertCount(2, DB::getQueryLog());

Model::automaticallyEagerLoadRelationships(false);

DB::disableQueryLog();
}

public function testRelationAutoloadWithCircularRelations()
{
$post = Post::create();
$comment1 = $post->comments()->create(['parent_id' => null]);
$comment2 = $post->comments()->create(['parent_id' => $comment1->id]);
$post->likes()->create();

DB::enableQueryLog();

$post->withRelationshipAutoloading();
$comment = $post->comments->first();
$comment->setRelation('post', $post);

$this->assertCount(1, $post->likes);

$this->assertCount(2, DB::getQueryLog());

DB::disableQueryLog();
}

public function testRelationAutoloadWithChaperoneRelations()
{
Model::automaticallyEagerLoadRelationships();

$post = Post::create();
$comment1 = $post->comments()->create(['parent_id' => null]);
$comment2 = $post->comments()->create(['parent_id' => $comment1->id]);
$post->likes()->create();

DB::enableQueryLog();

$post->load('commentsWithChaperone');

$this->assertCount(1, $post->likes);

$this->assertCount(2, DB::getQueryLog());

Model::automaticallyEagerLoadRelationships(false);

DB::disableQueryLog();
}

public function testRelationAutoloadVariousNestedMorphRelations()
Expand Down Expand Up @@ -163,6 +211,8 @@ public function testRelationAutoloadVariousNestedMorphRelations()
$this->assertCount(2, $videos);
$this->assertTrue($videoLike->relationLoaded('likeable'));
$this->assertTrue($videoLike->likeable->relationLoaded('commentable'));

DB::disableQueryLog();
}
}

Expand Down Expand Up @@ -197,6 +247,11 @@ public function comments()
return $this->morphMany(Comment::class, 'commentable');
}

public function commentsWithChaperone()
{
return $this->morphMany(Comment::class, 'commentable')->chaperone();
}

public function likes()
{
return $this->morphMany(Like::class, 'likeable');
Expand Down