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.

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

[8.x] Allow use of associative arrays as only argument to withAggregate, withMin, etc. #35073

Closed
wants to merge 1 commit into from
Closed

[8.x] Allow use of associative arrays as only argument to withAggregate, withMin, etc. #35073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
153 changes: 95 additions & 58 deletions src/Illuminate/Database/Eloquent/Concerns/QueriesRelationships.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,14 @@ public function orWhereDoesntHaveMorph($relation, $types, Closure $callback = nu
}

/**
* Add subselect queries to include an aggregate value for a relationship.
* Add subselect queries to include aggregate values for relationships.
*
* @param mixed $relations
* @param string $column
* @param string $function
* @return $this
*/
public function withAggregate($relations, $column, $function = null)
public function withAggregate($relations, $column = null, $function = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driesvints Thanks for looking into this. I'm sorry if I was on the wrong track here. I assumed making a required parameter optional would not be considered a breaking change. I think that nothing would actually break if it kept using the current syntax.

Copy link
Member

Choose a reason for hiding this comment

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

https://3v4l.org/VNAms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driesvints Got it. Thank you very much for clarifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driesvints Should I change the title to [9.x]? Sorry, I just started contributing recently.

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to submit a new PR to the master branch for this.

Copy link
Member

Choose a reason for hiding this comment

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

you need to update the phpdoc too

{
if (empty($relations)) {
return $this;
Expand All @@ -368,63 +368,100 @@ public function withAggregate($relations, $column, $function = null)

$relations = is_array($relations) ? $relations : [$relations];

foreach ($this->parseWithRelations($relations) as $name => $constraints) {
// First we will determine if the name has been aliased using an "as" clause on the name
// and if it has we will extract the actual relationship name and the desired name of
// the resulting column. This allows multiple aggregates on the same relationships.
$segments = explode(' ', $name);
foreach ($relations as $name => $constraints) {
$this->applyAggregateSubSelect($name, $constraints, $column, $function);
}

unset($alias);
return $this;
}

if (count($segments) === 3 && Str::lower($segments[1]) === 'as') {
[$name, $alias] = [$segments[0], $segments[2]];
}
/**
* Add a specific subselect query for a withAggregate for a relation.
*
* @param int|string $name
* @param string|callable $constraints
* @param string|null $column
* @param string|null $function
* @return $this
*/
protected function applyAggregateSubSelect($name, $constraints, $column = null, $function = null)
{
if (is_numeric($name)) {
$name = $constraints;
$constraints = $function ?? static function () {
//
};
}

$relation = $this->getRelationWithoutConstraints($name);
if (is_string($constraints)) {
$function = $constraints;
$constraints = static function () {
//
};
}

if ($function) {
$expression = sprintf('%s(%s)', $function, $this->getQuery()->getGrammar()->wrap(
$column === '*' ? $column : $relation->getRelated()->qualifyColumn($column)
));
} else {
$expression = $column;
}
// First we will determine if the name has been aliased using an "as" clause on the name
// and if it has we will extract the actual relationship name and the desired name of
// the resulting column. This allows multiple aggregates on the same relationships.
$segments = explode(' ', $name);

if (count($segments) === 3 && Str::lower($segments[1]) === 'as') {
[$name, $alias] = [$segments[0], $segments[2]];
}

// Here, we will grab the relationship sub-query and prepare to add it to the main query
// as a sub-select. First, we'll get the "has" query and use that to get the relation
// sub-query. We'll format this relationship name and append this column if needed.
$query = $relation->getRelationExistenceQuery(
$relation->getRelated()->newQuery(), $this, new Expression($expression)
)->setBindings([], 'select');
// Next we will check if a column has been specified using a colon and if it has we will
// use it instead of the $column parameter of this method. This allows aggregating or
// counting on a specific column of a relationship using a convenient array syntax.
if (Str::contains($name, ':')) {
[$name, $column] = explode(':', $name, 2);
} else {
$column = $column ?: '*';
}

$query->callScope($constraints);
$relation = $this->getRelationWithoutConstraints($name);

$query = $query->mergeConstraintsFrom($relation->getQuery())->toBase();
if ($function) {
$expression = sprintf('%s(%s)', $function, $this->getQuery()->getGrammar()->wrap(
$column === '*' ? $column : $relation->getRelated()->qualifyColumn($column)
));
} else {
$expression = $column;
}

// If the query contains certain elements like orderings / more than one column selected
// then we will remove those elements from the query so that it will execute properly
// when given to the database. Otherwise, we may receive SQL errors or poor syntax.
$query->orders = null;
$query->setBindings([], 'order');
// Here, we will grab the relationship sub-query and prepare to add it to the main query
// as a sub-select. First, we'll get the "has" query and use that to get the relation
// sub-query. We'll format this relationship name and append this column if needed.
$query = $relation->getRelationExistenceQuery(
$relation->getRelated()->newQuery(), $this, new Expression($expression)
)->setBindings([], 'select');

if (count($query->columns) > 1) {
$query->columns = [$query->columns[0]];
$query->bindings['select'] = [];
}
$query->callScope($constraints);

// Finally, we will make the proper column alias to the query and run this sub-select on
// the query builder. Then, we will return the builder instance back to the developer
// for further constraint chaining that needs to take place on the query as needed.
$alias = $alias ?? Str::snake(
preg_replace('/[^[:alnum:][:space:]_]/u', '', "$name $function $column")
);
$query = $query->mergeConstraintsFrom($relation->getQuery())->toBase();

$this->selectSub(
$function ? $query : $query->limit(1),
$alias
);
// If the query contains certain elements like orderings / more than one column selected
// then we will remove those elements from the query so that it will execute properly
// when given to the database. Otherwise, we may receive SQL errors or poor syntax.
$query->orders = null;
$query->setBindings([], 'order');

if (count($query->columns) > 1) {
$query->columns = [$query->columns[0]];
$query->bindings['select'] = [];
}

// Finally, we will make the proper column alias to the query and run this sub-select on
// the query builder. Then, we will return the builder instance back to the developer
// for further constraint chaining that needs to take place on the query as needed.
$alias = $alias ?? Str::snake(
preg_replace('/[^[:alnum:][:space:]_]/u', '', "$name $function $column")
);

$this->selectSub(
$function ? $query : $query->limit(1),
$alias
);

return $this;
}

Expand All @@ -442,49 +479,49 @@ public function withCount($relations)
/**
* Add subselect queries to include the max of the relation's column.
*
* @param string $relation
* @param string|array $relations
* @param string $column
* @return $this
*/
public function withMax($relation, $column)
public function withMax($relations, $column = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change to the method signature. All of these method signature changes below are breaking changes as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods did support arrays as the first parameter before, too. It was just not documented. But you might be more concerned about the second parameter becoming optional. As I mentioned in the comment above, nothing should break if it keeps using the former way implemented in #34965. But maybe I have a wrong understanding of what a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

https://3v4l.org/VNAms

{
return $this->withAggregate($relation, $column, 'max');
return $this->withAggregate($relations, $column, 'max');
}

/**
* Add subselect queries to include the min of the relation's column.
*
* @param string $relation
* @param string|array $relations
* @param string $column
* @return $this
*/
public function withMin($relation, $column)
public function withMin($relations, $column = null)
{
return $this->withAggregate($relation, $column, 'min');
return $this->withAggregate($relations, $column, 'min');
}

/**
* Add subselect queries to include the sum of the relation's column.
*
* @param string $relation
* @param string|array $relations
* @param string $column
* @return $this
*/
public function withSum($relation, $column)
public function withSum($relations, $column = null)
{
return $this->withAggregate($relation, $column, 'sum');
return $this->withAggregate($relations, $column, 'sum');
}

/**
* Add subselect queries to include the average of the relation's column.
*
* @param string $relation
* @param string|array $relations
* @param string $column
* @return $this
*/
public function withAvg($relation, $column)
public function withAvg($relations, $column = null)
{
return $this->withAggregate($relation, $column, 'avg');
return $this->withAggregate($relations, $column, 'avg');
}

/**
Expand Down
152 changes: 152 additions & 0 deletions tests/Database/DatabaseEloquentBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Illuminate\Database\Eloquent\RelationNotFoundException;
use Illuminate\Database\Eloquent\SoftDeletes;
use Illuminate\Database\Query\Builder as BaseBuilder;
use Illuminate\Database\Query\Expression;
use Illuminate\Database\Query\Grammars\Grammar;
use Illuminate\Database\Query\Processors\Processor;
use Illuminate\Support\Carbon;
Expand Down Expand Up @@ -865,6 +866,157 @@ public function testWithCountMultipleAndPartialRename()
$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (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_bar", (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 testWithCountAndColon()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->withCount('foo:completed_at');

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select count("eloquent_builder_test_model_close_related_stubs"."completed_at") 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_completed_at" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

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

$builder = $model->withCount('foo:completed_at as foos_completed');

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select count("eloquent_builder_test_model_close_related_stubs"."completed_at") 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 "foos_completed" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

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

$builder = $model->withMin('foo:price');

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select min("eloquent_builder_test_model_close_related_stubs"."price") 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_min_price" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

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

$builder = $model->withMin([
'foo:bar as big_bars' => function ($query) {
$query->where('bar', '>', 10);
},
])
->where('foo', 20);

$this->assertSame(
'select "eloquent_builder_test_model_parent_stubs".*'
.', (select min("eloquent_builder_test_model_close_related_stubs"."bar") 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 "bar" > ?) as "big_bars"'
.' from "eloquent_builder_test_model_parent_stubs"'
.' where "foo" = ?',
$builder->toSql()
);
$this->assertEquals([10, 20], $builder->getBindings());
}

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

$sqls = collect([
$model->withAggregate('foo', 'price', 'min'),
$model->withMin('foo', 'price'),
$model->withMin('foo:price'),
$model->withMin(['foo:price']),
$model->withMin(['foo:price' => function () {
//
}]),
$model->withMin(['foo:price as foo_min_price']),
])
->map(function ($query) {
return $query->toSql();
});

$this->assertCount(1, $sqls->unique()->all());
$this->assertSame(
'select "eloquent_builder_test_model_parent_stubs".*'
.', (select min("eloquent_builder_test_model_close_related_stubs"."price") 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_min_price"'
.' from "eloquent_builder_test_model_parent_stubs"',
$sqls[0]
);
}

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

$builder = $model->withMin(['foo:foo', 'address:bar', 'activeFoo:baz']);

$this->assertSame(
'select "eloquent_builder_test_model_parent_stubs".*'
.', (select min("eloquent_builder_test_model_close_related_stubs"."foo") 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_min_foo"'
.', (select min("eloquent_builder_test_model_close_related_stubs"."bar") 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 "address_min_bar"'
.', (select min("eloquent_builder_test_model_close_related_stubs"."baz") 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 "active" = ?) as "active_foo_min_baz"'
.' from "eloquent_builder_test_model_parent_stubs"',
$builder->toSql()
);
}

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

$builder = $model->select('id')->withAggregate([
'foo' => 'count',
'foo:* as aliased' => 'count',
'foo:price' => 'sum',
'foo:created_at' => 'min',
'foo as complex_sum' => function ($query) {
$query->select(new Expression('SUM(price*quantity)'));
},
]);

$this->assertSame(
'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"'
.', (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 "aliased"'
.', (select sum("eloquent_builder_test_model_close_related_stubs"."price") 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_sum_price"'
.', (select min("eloquent_builder_test_model_close_related_stubs"."created_at") 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_min_created_at"'
.', (select SUM(price*quantity) 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" limit 1) as "complex_sum"'
.' from "eloquent_builder_test_model_parent_stubs"',
$builder->toSql()
);
}

public function testWithSumAndGlobalScope()
{
$model = new EloquentBuilderTestModelParentStub;
EloquentBuilderTestModelCloseRelatedStub::addGlobalScope('withXCountX', function ($query) {
return $query->addSelect('id')->where('is_published', true);
});

$builder = $model->select('id')->withSum([
'foo:weight',
'foo:price',
'foo:bar as aliased' => function ($query) {
$query->where('bar', 'baz');
},
'foo as complex_sum' => function ($query) {
$query->select(new Expression('SUM(price*quantity)'));
},
]);

// Remove the global scope so it doesn't interfere with any other tests
EloquentBuilderTestModelCloseRelatedStub::addGlobalScope('withXCountX', function ($query) {
//
});

$this->assertSame(
'select "id"'
.', (select sum("eloquent_builder_test_model_close_related_stubs"."weight") 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 "is_published" = ?) as "foo_sum_weight"'
.', (select sum("eloquent_builder_test_model_close_related_stubs"."price") 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 "is_published" = ?) as "foo_sum_price"'
.', (select sum("eloquent_builder_test_model_close_related_stubs"."bar") 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 "bar" = ? and "is_published" = ?) as "aliased"'
.', (select SUM(price*quantity) 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 "is_published" = ?) as "complex_sum"'
.' from "eloquent_builder_test_model_parent_stubs"',
$builder->toSql()
);
}

public function testHasWithConstraintsAndHavingInSubquery()
{
$model = new EloquentBuilderTestModelParentStub;
Expand Down