[5.5] Add Eloquent Features: withSum,withMax,withMin,withAvg etc. - #19363
[5.5] Add Eloquent Features: withSum,withMax,withMin,withAvg etc.#19363xuqihua wants to merge 3 commits into
Conversation
|
` Example: |
|
A pipe is an interesting character to be using for such a thing 😑 |
|
I was thinking the same. A comma might make more sense in this case. |
|
Why isn't dot applicable here? |
|
@decadence the dot is used for nested relationships |
|
Which are not supported by withCount and these methods. |
jstoone
left a comment
There was a problem hiding this comment.
Really like the feature! 👌
Just a few comments, but nicely done. 🎉
| if (is_null($this->query->columns)) { | ||
| $this->query->select([$this->query->from.'.*']); | ||
| } | ||
|
|
There was a problem hiding this comment.
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(); | ||
|
|
There was a problem hiding this comment.
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.
|
Maybe use the colon This is also used in the rules for example |
|
@decadence |
|
I don't understand how it conficts here if these methods don't support nested relations. |
|
I don't think I'm going to add this for now. It may be macroable? |
|
Not sure why this issue didn't reference #16815. That seems to solve the problem with a more sensible Unfortunately all three PRs (#16815, #18303, and #19363) seem to lack decent unit tests (all I see are assertions on the final generated queries). |
|
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 I do believe that |
|
Just one more comment: it might also be worth implementing a This converts |
#18303
Add feature to 5.5.