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.5] Maintain original order of collection items with equal values when using sortBy() #21270

Merged
merged 3 commits into from Sep 20, 2017

Conversation

Projects
None yet
3 participants
@damiani
Contributor

damiani commented Sep 20, 2017

(Revisits #21214, which was reverted in #21255)

This PR implements stable sort for sortBy(), as discussed in #21214, but approaches it another way that doesn't break when passed sort options like SORT_STRING, SORT_NUMBER',SORT_NATURAL`, etc.

The result: sortBy() will now preserve the original order of collection items which have identical values, and will work correctly with all sort options.

Fixes #21253


Coincidentally, the instability of sortBy() also seems to be the cause of issues arising from the recent addition of route fallbacks, which ended up re-sorting routes in an unpredictable manner. So this PR should have the side-effect of fixing #21257 and #21258.


@themsaid

This comment has been minimized.

Show comment
Hide comment
@themsaid

themsaid Sep 20, 2017

Member

@damiani route matching on dev-master doesn't use sorting at all, so this PR can be merged separately in a later release.

Member

themsaid commented Sep 20, 2017

@damiani route matching on dev-master doesn't use sorting at all, so this PR can be merged separately in a later release.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 20, 2017

Member

I got several errors and several test failures when running this locally on PHP 7.1. Here is one of them:

  1. Illuminate\Tests\Routing\RoutingRouteTest::testOptionsResponsesAreGeneratedByDefault
    ErrorException: array_multisort(): Argument #1 is expected to be an array or a sort flag

/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Support/Collection.php:1358
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/RouteCollection.php:192
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/RouteCollection.php:164
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/Router.php:599
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/Router.php:578
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/Router.php:564
/Users/taylor/Documents/Code/Laravel/framework/tests/Routing/RoutingRouteTest.php:371

Member

taylorotwell commented Sep 20, 2017

I got several errors and several test failures when running this locally on PHP 7.1. Here is one of them:

  1. Illuminate\Tests\Routing\RoutingRouteTest::testOptionsResponsesAreGeneratedByDefault
    ErrorException: array_multisort(): Argument #1 is expected to be an array or a sort flag

/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Support/Collection.php:1358
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/RouteCollection.php:192
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/RouteCollection.php:164
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/Router.php:599
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/Router.php:578
/Users/taylor/Documents/Code/Laravel/framework/src/Illuminate/Routing/Router.php:564
/Users/taylor/Documents/Code/Laravel/framework/tests/Routing/RoutingRouteTest.php:371

@damiani

This comment has been minimized.

Show comment
Hide comment
@damiani

damiani Sep 20, 2017

Contributor

Hmm, all tests, including testOptionsResponsesAreGeneratedByDefault and all RoutingRouteTests, are passing for me and passed in Travis. I'll try to reproduce, but I suspect initializing $values to ensure it's an array will solve it.

Contributor

damiani commented Sep 20, 2017

Hmm, all tests, including testOptionsResponsesAreGeneratedByDefault and all RoutingRouteTests, are passing for me and passed in Travis. I'll try to reproduce, but I suspect initializing $values to ensure it's an array will solve it.

@taylorotwell taylorotwell merged commit 0c024f0 into laravel:5.5 Sep 20, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment