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

Fixes issue for ordering by a relationship. #1926

Open
wants to merge 3 commits into
base: 8.0
Choose a base branch
from
Open

Conversation

Nathanw
Copy link

@Nathanw Nathanw commented Nov 27, 2018

This fixes an issue when ordering by a HasMany relationship due to no selects being on the query and overriding the original models id with the id from the relationship.

*
* @param array $selects
*/
public function applySelects(array $selects)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use protected function. Thanks!

@yajra
Copy link
Owner

yajra commented Nov 28, 2018

The PR looks good and tests are passing. Will check this asap.

BTW, if possible, can you add a test for this scenario? Thanks! 🍻

@yajra
Copy link
Owner

yajra commented Jan 4, 2019

@Nathanw I just realized after testing this that allowing sort on HasMany relationship would yield to multiple duplicate records since we are not grouping on sql level.

Duplicate records when sorted via User -> HasMany-> Post
image

On the other hand, I think it does fix sorting on HasMany relationship.

Do you think we should merge this and allow the unnecessary duplicate results?

Any feedbacks from other users?

@OzanKurt
Copy link
Contributor

OzanKurt commented Oct 30, 2019

@yajra Almost every relation cause duplicate results. It's always correct to groupBy the query inside your DataTable class manually. I encountered this problem over 100 times.

Nowadays I always include $query->groupBy('my_table'); just before I return the results.

I feel like this should be merged and groupBy thing should be mentioned as a "Good Practice" in the Docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants