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

[5.7] Add withAggregate #25319

Closed
wants to merge 18 commits into from
Closed

[5.7] Add withAggregate #25319

wants to merge 18 commits into from

Conversation

spyp
Copy link

@spyp spyp commented Aug 24, 2018

No description provided.

spyp and others added 9 commits August 22, 2018 10:47
add withSum to framework . in the past people try to add withMax withMin withAvg withSum and ... together and implements withCount
i think its should be separte because in withSum withAvg and... we should determine what column but in withCount we dont 
so i add withSum if you think its ok( i test it ) add withAvg and .... . im not pro to do that soory for my bad english
add Tests for withSum
I change some methods and add withSum withCount withMin withMax withAvg that use witheAggregate that support all aggregate
i add somet migrations and seed in test/migration and test against real database (i'm not sure its right way but i dont have better idea)
hope its working .
@spyp
Copy link
Author

spyp commented Aug 24, 2018

reference to : #25290

@jmarcher
Copy link
Contributor

Can you check why do your tests fail?

@spyp
Copy link
Author

spyp commented Aug 24, 2018

Hi. can you tell me what error you got because i test them before PR . by the way as i mentioned i think my tests specially migrations and seed tables is hard for test . and i don't have idea that how can i create proper test against database.anyway if you can tell me you have syntax error or fail assertion.

@jmarcher
Copy link
Contributor

@spyp
Copy link
Author

spyp commented Aug 24, 2018

you are right i didn't check travis. i will try to fix tests

@GrahamCampbell GrahamCampbell changed the title [5.6] add withAggregate [5.7] Add withAggregate Sep 3, 2018
@GrahamCampbell GrahamCampbell changed the base branch from 5.6 to 5.7 September 3, 2018 09:40
@taylorotwell
Copy link
Member

Since last time we tried this it caused a lot of problems, I want to this vetted as a package first. Both the builder and relation class are Macroable, so you should be able to macro these methods into the classes. If that package works without issue for people I will merge this into the core.

@arxeiss
Copy link
Contributor

arxeiss commented Oct 6, 2018

This would be really nice feature. Hope it will get merged one day.

But for now, I found little workaround, which works in Laravel 5.7. Not probably the best solution, but it does the work

User::withCount(['terms as term_sum' => function($query){
    $query->select(DB::raw('SUM(id)'));
}])

@dwightwatson
Copy link
Contributor

I actually ended up breaking this out into a package: watson/aggregate - wasn't sure if @spyp was interested in doing it and hope it's alright with him - to use until it's reconsidered for the core.

@deleugpn
Copy link
Contributor

@arxeiss mindblowing hacking. That workaround is amazing.

@Tencryn
Copy link

Tencryn commented Oct 15, 2019

@dwightwatson absolute life saver. I needed withAvg and decided to give myself a headache by doing some crazy selectRaw as I needed to support polymorphic relations. I decided to search as I couldn't be the only person to think about needing a method like this and here I am. Good work, I hope it will make it's way into core at some point in the future.

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.

8 participants