[8.x] Add withColumn() to QueriesRelationships to support min, max, sum and… #34965

khalilst · 2020-10-25T19:58:53Z

I add a method named withColumn (copied/edited from withCount) method to support more SQL aggregation functions like min, max, sum, avg over relationships, e.g.

Post::withCount('comments');
Post::withMax('comments', 'created_at');
Post::withMin('comments', 'created_at');
Post::withSum('comments', 'foo');
Post::withAvg('comments', 'foo');

Also, the withCount methods uses withColumn to avoid duplicated code and it works with backward-compatibility.
However, it is possible to call other/custom SQL function using withColumn, e.g.

Post::withColumn('comments', 'created_at', 'distinct');
Post::withColumn('comments', 'content', 'length');
Post::withColumn('comments', 'created_at', 'custom_function');

In addition to, it is possible to add relationship's column to selected columns without SQL function, e.g.

Comment::withColumn('post', 'title');
Post::withColumn('comments', 'content');

Both are working. Please consider the sub-query limited to have only one result.

driesvints · 2020-10-26T10:11:41Z

This PR unfortunately contains breaking changes to a method signature. It also contains unrelated style changes to docblocks etc. Please send to master.

khalilst · 2020-10-26T13:06:28Z

@driesvints
I think there was a misunderstanding because of the similarity between withCount() and withColumn() source due to git diff.
The withCount() method has no changes to the method signature and it is completely the same as the original source.

    /**
     * Add subselect queries to count the relations.
     *
     * @param  mixed  $relations
     * @return $this
     */
    public function withCount($relations)
    {
        return $this->withColumn(is_array($relations) ? $relations : func_get_args(), '*', 'count');
    }

Just the body of the method has been changed.
And the changes to docblocks are completely related, because those blocks are not inside withCount() method, it is inside the general-purpose withColumn() method which is used by withCount, withMin, winMax, .....

@taylorotwell
Please review this PR again because of the mentiond misunderstandings.

driesvints · 2020-10-26T13:23:50Z

@khalilst right, thanks for clarifying.

… avg similar to withCount

taylorotwell · 2020-10-27T13:37:05Z

@khalilst did you follow this for this implementation? https://github.com/dwightwatson/aggregate/blob/master/src/AggregateServiceProvider.php

khalilst · 2020-10-27T15:28:14Z

@taylorotwell
Hmmm, honestly no, I did not know about that. However, seems there are similarities, but:
In fact, I was inspired from Builder's sum, min, max, average methods located in src/Illuminate/Database/Query/Builder.php.

I addition to those methods and support for other aggregation methods, I have add capability to return single column without aggregation which are not available in your given link.
Please consider the limit 1 in withColumn.

taylorotwell · 2020-10-27T15:35:04Z

OK, after reviewing the implementation by @dwightwatson do you see any problems with this implementation?

khalilst · 2020-10-27T17:19:23Z

@taylorotwell
If you are asking me, I have to say no, there is no problem with this PR's implementation.
The withCount() automatic tests were successful.

taylorotwell · 2020-10-29T13:31:42Z

@khalilst this PR forgot to add loadAvg, loadMin, etc. to Eloquent/Collection and Eloquent/Model. Note there are also methods for loadMorphCount.

khalilst · 2020-10-29T16:48:10Z

@taylorotwell
All right. I will add those methods to Eloquent/Collection, Eloquent/Model, and loadMorth....

kanidjar · 2020-10-29T19:11:22Z

hi @khalilst, @taylorotwell, the new withCount has broken our app.

We are using withCount inside a globalScope:

    public static function bootHasAudiences(): void
    {
            static::addGlobalScope(
                'has_audiences',
                function (Builder $query): void {
                    // sort by audience
                    $query->withCount('audiences')->orderBy('audiences_count', 'desc');
                }
            );
        }
    }
    
    public function audiences()
    {
        return $this->belongsToMany(Audience::class)->using(AudiencePivot::class);
    }

Error: Maximum function nesting level of '256' reached, aborting!

before this merge, everything was working correctly.

khalilst · 2020-10-29T19:14:57Z

@kanidjar
I will check it right now.

taylorotwell · 2020-10-29T19:17:32Z

@khalilst let me know soon if you see the problem / fix - otherwise I will have to revert everything.

khalilst · 2020-10-29T19:44:10Z

@taylorotwell
I am already working on this. I will let you know the result.

khalilst · 2020-10-29T20:08:08Z

@taylorotwell I tried different projects with the given code by @kanidjar and I have seen no problem.

@kanidjar You have edited your code several times. Are you sure there was not missing code here?
I have to metion that withCount() tests passed successfully.
Please paste the call-stack here.

kanidjar · 2020-10-29T20:25:11Z

@khalilst here it is: https://gist.github.com/kanidjar/5594c42ced9b7494239bf28733f243a0

I've edited the code to remove all unrelated code.

A simple Item::all() with the global scope enabled is enough.

khalilst · 2020-10-29T20:31:17Z

@kanidjar Thank you. I'm trying to find out the problem.

khalilst · 2020-10-29T21:13:25Z

@taylorotwell @kanidjar I found the problem raised from line 385 at QueriesRelationships.php.
It was not in this PR, I think it was from code modifications/optimizations have done after merging by @sebastiaanluca.

I corrected this line and commit to open related PR #35029.

kanidjar · 2020-10-29T21:22:57Z

Thank you very much for investigating. So, according to you, the issue comes from this PR #35004

taylorotwell · 2020-10-29T21:23:51Z

@khalilst we need the fix in a separate PR. Also, why does that cause the problem. The change @sebastiaanluca made is pretty valid that we would want to wrap the column names.

kanidjar · 2020-10-29T21:30:38Z

@taylorotwell I can confirm that applying the fix locally fixes the issue.

khalilst · 2020-10-29T21:31:47Z

@taylorotwell Please take a look at the stack trace given by @kanidjar here.
You can see the exact line number 385 which the wrapping done here (at the bottom of gist).
I tested in my local environment and after modifying this line the reported issue resolved.
I will create another PR for this bugfix.

@kanidjar Yes, you are right.

taylorotwell · 2020-10-29T21:38:14Z

We can call $this->getQuery()->getGrammar()->wrap() instead.

taylorotwell · 2020-10-29T21:38:41Z

Pushed to 8.x. Please remove your fix from this from your other PR.

taylorotwell · 2020-10-29T21:39:27Z

@khalilst can you confirm this commit fixes your problem if you apply the change directly in your vendor directory: 20b0c6e

khalilst · 2020-10-29T21:42:11Z

@taylorotwell Yes, it does.

I'm gonna remove the bugfix from other PR.

taylorotwell · 2020-10-29T22:02:11Z

Released as 8.12.3.

robsontenorio · 2020-10-30T18:24:32Z

@khalilst

The withCount method allow us adding constraints. How can we achieve that with withMax, withMin ?

$posts = App\Models\Post::withCount(['votes', 'comments' => function (Builder $query) {
    $query->where('content', 'like', 'foo%');
}])->get();

khalilst · 2020-10-30T21:43:50Z

@robsontenorio
Same as withCount. Just consider to pass the second argument $column:

Post::withMax(['votes', 'comments' => function (Builder $query) {
    $query->where('content', 'like', 'someting%');
}], 'column_name')->get();

Note: If you are using with{Max, Min, Sum, Avg} for an array of relations, the column should exist in all relations.
In the above example, the column_name should be available in votes & comments,

robsontenorio · 2020-10-30T22:17:00Z

@khalilst thank you! The trick was second param column_name , not present in withCount (at least on laravel docs)

khalilst · 2020-10-30T23:15:24Z

@robsontenorio You are welcome. I will create a PR to Laravel Doc asap.

Alexmg86 · 2020-11-03T13:17:22Z

@taylorotwell
Good afternoon!
I am a big fan of your work and in particular laravel is very upset. That year I tried to suggest you new things that were uploaded in this commit #32474 but you rejected it and offered to write a package.
Which is what I did https://github.com/Alexmg86/laravel-sub-query
But this person just took all my work and added here and was accepted?
Very disappointing.

driesvints · 2020-11-03T13:25:34Z

@Alexmg86 sometimes we reconsider feature requests. Don't be disappointed that your PR wasn't accepted at the time. Be happy that it's now finally in the core of the framework.

driesvints · 2020-11-03T13:27:00Z

@Alexmg86 also, looking at your PR it wasn't closed because we didn't accept it. It was closed because it lacked a thorough explanation of the feature and tests.

Alexmg86 · 2020-11-03T13:29:34Z

@driesvints But you could have said it, but a person just stole my work and this is at least bad. I'm really very upset

driesvints · 2020-11-03T13:42:18Z

Can you point out which parts the contributor stole from you?

Alexmg86 · 2020-11-03T13:47:54Z

@driesvints Methods related to aggregation withMax, withMin, withSum, withAvg. You can see the dates of my commits, ideas.

driesvints · 2020-11-03T13:52:47Z

Please link to them.

Alexmg86 · 2020-11-03T13:55:14Z

@driesvints how to do it?

driesvints · 2020-11-03T14:04:36Z

@Alexmg86 select the links to the pieces of your code and paste them here so we can compare them.

khalilst · 2020-11-03T14:36:44Z

@taylorotwell
Good afternoon!
I am a big fan of your work and in particular laravel is very upset. That year I tried to suggest you new things that were uploaded in this commit #32474 but you rejected it and offered to write a package.
Which is what I did https://github.com/Alexmg86/laravel-sub-query
But this person just took all my work and added here and was accepted?
Very disappointing.

@Alexmg86 I have never seen your code and I can not understand your insulting words without any proof.
You think you are the only person in this world that knows about aggregation.
I have just used Laravel's latest source the old withCount method and changed it to support other aggregation funcitons and single column as a result.

Alexmg86 · 2020-11-03T14:41:32Z

@khalilst ok. I think so. Maybe you are right

khalilst · 2020-11-03T14:46:32Z

@Alexmg86 Sure, but definitely you were wrong

driesvints · 2020-11-03T14:48:46Z

I'm going to close this off because I believe nothing more constructive can come out of this discussion.

GrahamCampbell changed the title ~~Add withColumn() to QueriesRelationships to support min, max, sum and…~~ [8.x] Add withColumn() to QueriesRelationships to support min, max, sum and… Oct 25, 2020

driesvints closed this Oct 26, 2020

driesvints reopened this Oct 26, 2020

Add withColumn() to QueriesRelationships to support min, max, sum and…

95d0c9a

… avg similar to withCount

taylorotwell merged commit 95d0c9a into laravel:8.x Oct 27, 2020

khalilst mentioned this pull request Oct 29, 2020

[8.x] Add loadAggregate() and load[min(), max(), sum() & avg()] methods to … #35029

Merged

paras-malhotra mentioned this pull request Nov 2, 2020

withCount not working as expected #35059

Closed

dbakan mentioned this pull request Nov 3, 2020

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

Closed

laravel locked and limited conversation to collaborators Nov 3, 2020

[8.x] Add withColumn() to QueriesRelationships to support min, max, sum and… #34965

[8.x] Add withColumn() to QueriesRelationships to support min, max, sum and… #34965

Conversation

khalilst commented Oct 25, 2020 • edited Loading

driesvints commented Oct 26, 2020

khalilst commented Oct 26, 2020

driesvints commented Oct 26, 2020

taylorotwell commented Oct 27, 2020

khalilst commented Oct 27, 2020

taylorotwell commented Oct 27, 2020

khalilst commented Oct 27, 2020

taylorotwell commented Oct 29, 2020

khalilst commented Oct 29, 2020

kanidjar commented Oct 29, 2020 • edited Loading

khalilst commented Oct 29, 2020

taylorotwell commented Oct 29, 2020

khalilst commented Oct 29, 2020

khalilst commented Oct 29, 2020

kanidjar commented Oct 29, 2020

khalilst commented Oct 29, 2020

khalilst commented Oct 29, 2020

kanidjar commented Oct 29, 2020

taylorotwell commented Oct 29, 2020

kanidjar commented Oct 29, 2020

khalilst commented Oct 29, 2020

taylorotwell commented Oct 29, 2020

taylorotwell commented Oct 29, 2020

taylorotwell commented Oct 29, 2020

khalilst commented Oct 29, 2020

taylorotwell commented Oct 29, 2020

robsontenorio commented Oct 30, 2020 • edited Loading

khalilst commented Oct 30, 2020

robsontenorio commented Oct 30, 2020

khalilst commented Oct 30, 2020

Alexmg86 commented Nov 3, 2020

driesvints commented Nov 3, 2020

driesvints commented Nov 3, 2020

Alexmg86 commented Nov 3, 2020

driesvints commented Nov 3, 2020 • edited Loading

Alexmg86 commented Nov 3, 2020

driesvints commented Nov 3, 2020

Alexmg86 commented Nov 3, 2020

driesvints commented Nov 3, 2020 • edited Loading

khalilst commented Nov 3, 2020

Alexmg86 commented Nov 3, 2020 • edited Loading

khalilst commented Nov 3, 2020

driesvints commented Nov 3, 2020

khalilst commented Oct 25, 2020 •

edited

Loading

kanidjar commented Oct 29, 2020 •

edited

Loading

robsontenorio commented Oct 30, 2020 •

edited

Loading

driesvints commented Nov 3, 2020 •

edited

Loading

driesvints commented Nov 3, 2020 •

edited

Loading

Alexmg86 commented Nov 3, 2020 •

edited

Loading