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.

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

wants to merge 1 commit into from

Conversation

dbakan
Copy link
Contributor

@dbakan dbakan commented Nov 3, 2020

This PR allows the use of an associative array as the only argument for the new Builder methods withAggregates, withSum, etc. introduced in #34965.

It lets you specify the targeted column using a colon relation:column as seen in Model::with('relation:id,name').

With the current implementation you need multiple calls if you want to sum over different columns:

Order::withSum('items', 'price')->withSum('items', 'weight');

With this change you may instead write

Order::withSum(['items:price', 'items:weight']);

This also enables counting on a specific column using withCount

Post::withCount('comments:approved_by as approved_comments_count');

// Might also be used to gain a possible performance benefit over count(*):
Post::withCount('comments:post_id as comments_count');

Some more examples:

// build sums over multiple relations and/or columns
Order::withSum([ 
   'items:weight as shipping_weight',
   'items:cost as total_price',
   'vouchers:price as total_vouchers',
   'refunds:value as total_refunds' => function($query) {
      $query->where('foo', 'bar');
   }
]);

// sum up the products by multiplying `price × quantity`
Order::withSum([ 
   'items as total_cost' => function($query) {
        $query->select(new Expression('sum(price*quantity)'));
    },
]);

// use string values to specify the individual aggregate functions to apply:
Order::withAggregate([ 
   'items' => 'count',
   'items:updated_at as oldest_item_update' => 'min',
   'items:updated_at as newest_item_update' => 'max',
   'items:price as total_price' => 'sum',
   'items:weight as total_weight' => 'sum',
   'items:weight as max_weight' => 'max',
]);

A note about optional and mandatory square brackets:

The implementation of withCount already differs from all the other withAggregate, withSum, withMin, ... :

// square brackets are optional for withCount
Order::withCount('relation_a', 'relation_b');
// square brackets are mandatory for all the others
Order::withSum(['relation_a', 'relation_b'], 'column'); // existing syntax
Order::withSum(['relation_a:column', 'relation_b:column']); // this PRs additional syntax

To prevent a breaking change, these differences still remain and exist unchanged.

Steps taken:

  • allow the $column param of withAggregate() to be null
  • stop using parseWithRelations() here as it trims of the :column part of the array keys and calls unwanted select() and addNestedWiths()
  • move inner for-loop logic from withAggregate to a new applyAggregateSubSelect() to allow changing $column, $function, $alias without side effects in the next iteration
  • add the new logic of this PR

I added tests to tests/Database/DatabaseEloquentBuilderTest.php but no integration tests yet.

I'd also like to kindly ask @khalilst for his thoughts on this, since he contributed a lot on this topic lately.

*
* @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.

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

* @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.

@taylorotwell
Copy link
Member

Why don't we just keep it with multiple calls for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants