jarektkaczyk / eloquence-base

base for the Eloquence extensions + Searchable
https://softonsofa.com
MIT License
77 stars 69 forks source link

Aggregate is failing with certain db settings #4

Open ragingdave opened 6 years ago

ragingdave commented 6 years ago

In the current Query Builder instance in this package, the aggregate method doesn't remove things like order at least like the latest laravel version does to remove errors around full group by.

I can run the same query twice using this package enabled and without and the resulting queries look like:

With:

select count(*) as aggregate from "manufacturers" order by "id" desc

Without:

SELECT count(*) as aggregate FROM "manufacturers"

This causes the query to fail on servers where this option is enabled, whereas base installs of laravel, can function properly with this option.

For reference, https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Query/Builder.php#L2421

ragingdave commented 5 years ago

Is this package still being maintained at all? I would think that a month's time would be enough to at least comment on things? The last change I suggested last time took close to 3 months (maybe more) to even have a response of some kind.

jarektkaczyk commented 5 years ago

thanks for feedback :+1: this method cannot be simply removed as original laravel's behavior works differently when we have subquery used in searchable

ragingdave commented 5 years ago

Shouldn't this be something that the Searchable package has then? If I'm not using it why should usage of something unrelated break?

jarektkaczyk commented 5 years ago

It shouldn't break indeed. Can you provide a sample code to reproduce exactly the issue?

ragingdave commented 5 years ago

I don't recall specifically what the code was but based on the resulting query I had here:

Model::orderBy('id')->count()

This would work in laravel but not under this package.

jarektkaczyk commented 5 years ago

I will give it a shot and make a fix where necessary! :clinking_glasses: