Skip to content

[5.2] Merge wheres to hasQuery for withCount() #13612

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.

Sign up for GitHub

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 5 commits into from
May 18, 2016
Merged

[5.2] Merge wheres to hasQuery for withCount() #13612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
788ba0b
Merge wheres to hasQuery for withCount()
barryvdh May 18, 2016
356e199
Add test for merged wheres in withCount
barryvdh May 18, 2016
03300b0
Add test for merged wheres in withCount
barryvdh May 18, 2016
9d68e54
Switch wheres/constraints, use toBase() instead of getQuery()
barryvdh May 18, 2016
4abb185
Add softDeletesIntegration test for withCount
barryvdh May 18, 2016
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
3 changes: 2 additions & 1 deletion src/Illuminate/Database/Eloquent/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,8 +1031,9 @@ public function withCount($relations)
);

call_user_func($constraints, $query);
Copy link
Contributor

@acasar acasar May 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barryvdh I think that call_user_func should come before mergeModelDefinedRelationWheresToHasQuery.

That's because you can potentially remove global scopes from the $query in the callback and mergeModelDefinedRelationWheresToHasQuery takes care of removing them from the $relation too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.
Should I also call toBase() instead of query, to apply the scopes back?

Copy link
Contributor

@acasar acasar May 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah you should. Nice catch!

So, line 1036 instead of $query->getQuery() it should be $query->toBase()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so I changed both, although I'm not really sure how to write an actual test for that specific case..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be very similar to this: https://github.com/laravel/framework/blob/5.2/tests/Database/DatabaseEloquentSoftDeletesIntegrationTest.php#L410:L427

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something like this? 4abb185

$this->mergeModelDefinedRelationWheresToHasQuery($query, $relation);

$this->selectSub($query->getQuery(), snake_case($name).'_count');
$this->selectSub($query->toBase(), snake_case($name).'_count');
}

return $this;
Expand Down
17 changes: 17 additions & 0 deletions tests/Database/DatabaseEloquentBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,18 @@ public function testWithCountAndSelect()
$this->assertEquals('select "id", (select count(*) from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_count" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWithCountAndMergedWheres()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->select('id')->withCount(['activeFoo' => function ($q) {
$q->where('bam', '>', 'qux');
}]);

$this->assertEquals('select "id", (select count(*) from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id" and "bam" > ? and "active" = ?) as "active_foo_count" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
$this->assertEquals(['qux', true], $builder->getBindings());
}

public function testWithCountAndContraintsAndHaving()
{
$model = new EloquentBuilderTestModelParentStub;
Expand Down Expand Up @@ -683,6 +695,11 @@ public function foo()
{
return $this->belongsTo('EloquentBuilderTestModelCloseRelatedStub');
}

public function activeFoo()
{
return $this->belongsTo('EloquentBuilderTestModelCloseRelatedStub', 'foo_id')->where('active', true);
}
}

class EloquentBuilderTestModelCloseRelatedStub extends Illuminate\Database\Eloquent\Model
Expand Down
37 changes: 37 additions & 0 deletions tests/Database/DatabaseEloquentSoftDeletesIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,43 @@ public function testWhereHasWithNestedDeletedRelationshipAndWithTrashedCondition
$this->assertEquals(1, count($users));
}

/**
* @group test
*/
public function testWithCountWithNestedDeletedRelationshipAndOnlyTrashedCondition()
{
$this->createUsers();

$abigail = SoftDeletesTestUser::where('email', 'abigailotwell@gmail.com')->first();
$post1 = $abigail->posts()->create(['title' => 'First Title']);
$post1->delete();
$post2 = $abigail->posts()->create(['title' => 'Second Title']);
$post3 = $abigail->posts()->create(['title' => 'Third Title']);

$user = SoftDeletesTestUser::withCount('posts')->orderBy('postsCount', 'desc')->first();
$this->assertEquals(2, $user->posts_count);

$user = SoftDeletesTestUser::withCount(['posts' => function ($q) {
$q->onlyTrashed();
}])->orderBy('postsCount', 'desc')->first();
$this->assertEquals(1, $user->posts_count);

$user = SoftDeletesTestUser::withCount(['posts' => function ($q) {
$q->withTrashed();
}])->orderBy('postsCount', 'desc')->first();
$this->assertEquals(3, $user->posts_count);

$user = SoftDeletesTestUser::withCount(['posts' => function ($q) {
$q->withTrashed()->where('title', 'First Title');
}])->orderBy('postsCount', 'desc')->first();
$this->assertEquals(1, $user->posts_count);

$user = SoftDeletesTestUser::withCount(['posts' => function ($q) {
$q->where('title', 'First Title');
}])->orderBy('postsCount', 'desc')->first();
$this->assertEquals(0, $user->posts_count);
}

public function testOrWhereWithSoftDeleteConstraint()
{
$this->createUsers();
Expand Down