Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Support sorting by multiple columns #49

Merged
merged 6 commits into from
Feb 24, 2019

Conversation

sumpygump
Copy link
Contributor

This adds the ability to sort by multiple columns.

I ran into a situation where I wanted to sort things by category and then name, but the original orderBy() only allowed sorting by one column. This update allows to call orderBy() multiple times and it will sort by the columns in the order that orderBy is called.

$result = $db->query()
    ->orderBy('category', 'DESC')
    ->orderBy('name', 'ASC');

I added tests to fully test this new feature as well.

@timothymarois
Copy link
Member

Nice Job! I Like it. Travis CI failed with php 5.6. Is there any way you could reconfigure to get the test to pass? Just trying to stay backward compatible.

@sumpygump
Copy link
Contributor Author

Yeah, I see it fails in PHP 5.6. I will take a look and see what I can do.

@sumpygump
Copy link
Contributor Author

Update: After looking into this I found the problem was because the test where the data items were sorted by only one orderBy call was the failing test. Because of how I was asserting the results of this test, I was expecting the same results each time that the usort callback function was returning 0 when the items being tested were equal.

It turns out the problem is a difference in how the sort algorithm works in 5.6 vs 7.x, per the php website:
http://php.net/manual/en/migration70.incompatible.php

Sort order of equal elements
The internal sorting algorithm has been improved, what may result in different sort order of elements, which compare as equal, than before.

Note:
Don't rely on the order of elements which compare as equal; it might change anytime.

Based on this I will make an update to the test to account for this. The good news is the core functionality of the multiple orderBys is not something that is using PHP 7.x features only.

@timothymarois
Copy link
Member

@sumpygump This looks good. Thanks again for looking into it. I will merge this into the next update.

@timothymarois timothymarois merged commit 757919f into tmarois:1.0 Feb 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants