Skip to content

[5.5] Add Eloquent Features: withSum,withMax,withMin,withAvg etc. - #19363

Closed
xuqihua wants to merge 3 commits into
laravel:masterfrom
xuqihua:master
Closed

[5.5] Add Eloquent Features: withSum,withMax,withMin,withAvg etc.#19363
xuqihua wants to merge 3 commits into
laravel:masterfrom
xuqihua:master

Conversation

@xuqihua

@xuqihua xuqihua commented May 27, 2017

Copy link
Copy Markdown

#18303

Add feature to 5.5.

@xuqihua

xuqihua commented May 27, 2017

Copy link
Copy Markdown
Author

` Example:

$row = Post::withSum('comments|like')
    ->withMax('comments|like')
    ->withMin('comments|like')
    ->withAvg('comments|like')
    ->first();

$row->toArray()

like data: 1,2,3,4,5
expect:
[
    ...
    'comments_sum' => 15,
    'comments_max' => 5,
    'comments_min' => 1,
    'comments_avg' => 3,
];`

@JosephSilber

Copy link
Copy Markdown
Contributor

A pipe is an interesting character to be using for such a thing 😑

@tillkruss tillkruss changed the title Add Eloquent Features: withSum,withMax,withMin,withAvg etc. [5.5] Add Eloquent Features: withSum,withMax,withMin,withAvg etc. May 28, 2017
@tillkruss

Copy link
Copy Markdown
Contributor

I was thinking the same. A comma might make more sense in this case.

@decadence

Copy link
Copy Markdown
Contributor

Why isn't dot applicable here?

@mateusjatenee

Copy link
Copy Markdown
Contributor

@decadence the dot is used for nested relationships

@decadence

decadence commented May 29, 2017

Copy link
Copy Markdown
Contributor

Which are not supported by withCount and these methods.

@jstoone jstoone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really like the feature! 👌

Just a few comments, but nicely done. 🎉

if (is_null($this->query->columns)) {
$this->query->select([$this->query->from.'.*']);
}

@jstoone jstoone May 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The first comment in this review
I would remove the following comment, since the code speaks for it self. 👍


// set the default column as * or primary key
$column = ($function == 'count') ? '*' : $this->model->getKeyName();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Double negatives
You can avoid the double negative of not, not having the pipe by using:

if (Str::contains($name, '|')) { /* ... */ }

Pipe vs. comma
And also I would personally choose a comma as a separator, since I find the pipe communicates AND whereas the comma communicates more separation than the pipe. Hope that makes sense.

@marcvdm

marcvdm commented May 30, 2017

Copy link
Copy Markdown
Contributor

Maybe use the colon name:column

This is also used in the rules for example unique:posts - rule:column

@xuqihua

xuqihua commented May 31, 2017

Copy link
Copy Markdown
Author

@decadence
dot(.) is used in nested relation
any better suggestion?

@decadence

Copy link
Copy Markdown
Contributor

I don't understand how it conficts here if these methods don't support nested relations.
Anyway I'm fine with any separator.

@taylorotwell

Copy link
Copy Markdown
Member

I don't think I'm going to add this for now. It may be macroable?

@alexweissman

Copy link
Copy Markdown

Not sure why this issue didn't reference #16815. That seems to solve the problem with a more sensible , to separate the arguments, rather than combining them on a pipe.

Unfortunately all three PRs (#16815, #18303, and #19363) seem to lack decent unit tests (all I see are assertions on the final generated queries).

@alexweissman

Copy link
Copy Markdown

Actually, if separating out the relation and property names into two separate arguments is a problem (for example, when you want to apply constraints), what about the ~ character as a separator? There is precedent to using it for parent-child relationships, for example in CSS.

I do believe that : would be the best, but unfortunately that already appears to have special meaning, at least in Laravel 5.4: https://github.com/illuminate/database/blob/5.4/Eloquent/Builder.php#L1067

@alexweissman

Copy link
Copy Markdown

Just one more comment: it might also be worth implementing a withNumericAggregate method to parallel the numericAggregate method in Query\Builder: https://github.com/illuminate/database/blob/5.4/Query/Builder.php#L2032-L2059

This converts null sums/averages/etc to 0 and casts all aggregates to an appropriate int or float.

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.

9 participants