search

aarondfrancis / fast-paginate

A fast implementation of offset/limit pagination for Laravel.
MIT License
1.21k stars 56 forks source link

withCount() and an orderByDesc() results in a column not found exception #15

Closed realslimsutton closed 2 years ago

realslimsutton commented 2 years ago

Fixes #14

realslimsutton commented 2 years ago

I'm still investigating the repurcussions of using ->reorder() as that'll remove all ordering, not just withCount columns.

I'll also add tests, but currently all tests are failing as I don't have the databases setup.

realslimsutton commented 2 years ago

Now that tests are running, I can confirm ->reorder() definitely is not okay, as expected it removes ordering for all columns (including "non-count" columns). I'm looking for alternatives.

realslimsutton commented 2 years ago

My first solution wasn't going to work due to the above issue.

This solution will prevent the sub query from selecting all columns, but also propagate selecting specific columns. This fixes #14, and maybe even adds support for the "having" clause (although I haven't tested the latter and have left it being deferred).

aarondfrancis commented 2 years ago

@realslimsutton I appreciate you putting this together! I think we want to be a little more delicate about how we include subselects, as they can be quite expensive.

Can you check on #16 and let me know what you think?

realslimsutton commented 2 years ago

@realslimsutton I appreciate you putting this together! I think we want to be a little more delicate about how we include subselects, as they can be quite expensive.

Can you check on #16 and let me know what you think?

@aarondfrancis I can confirm #16 also fixes the error I was getting, and also returns an ordered collection as expected. Thanks!

aarondfrancis commented 2 years ago

Merged #16, closing this now. Thanks again for getting me started here!