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

[5.5] revert PR #21214 #21255

Merged
merged 1 commit into from
Sep 19, 2017
Merged

[5.5] revert PR #21214 #21255

merged 1 commit into from
Sep 19, 2017

Conversation

themsaid
Copy link
Member

As reported in #21253, this PR broke when any of the following options is used:

  • SORT_STRING
  • SORT_LOCALE_STRING
  • SORT_NATURAL

@taylorotwell taylorotwell merged commit febcb3c into laravel:5.5 Sep 19, 2017
@taylorotwell
Copy link
Member

@damiani

@damiani
Copy link
Contributor

damiani commented Sep 19, 2017

OK, will take a look and address.

@damiani
Copy link
Contributor

damiani commented Sep 19, 2017

I've identified a fix that works correctly with all sort options; I'll do some thorough testing to be certain, and resubmit this evening.

@yajra
Copy link
Contributor

yajra commented Sep 20, 2017

This PR broke routing with related issues #21257 #21258 and maybe #21263.

@@ -1348,7 +1348,7 @@ public function sortBy($callback, $options = SORT_REGULAR, $descending = false)
// function which we were given. Then, we will sort the returned values and
// and grab the corresponding values for the sorted keys from this array.
foreach ($this->items as $key => $value) {
$results[$key] = [$callback($value, $key), $key];
$results[$key] = $callback($value, $key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting this line fixes the route issues for me.

yajra added a commit to yajra/framework that referenced this pull request Sep 20, 2017
@damiani
Copy link
Contributor

damiani commented Sep 20, 2017

I think the root cause of route-related issues on 5.5.5 to 5.5.7 is actually b1e01e7, as part of the added support for fallback routes (#21234).

The matchAgainstRoutes() method sorts all routes by isFallback, which, because sortBy doesn't perform a stable sort, has the unintended side-effect of reordering routes in an unpredictable order.

@themsaid's PR #21261 fixes that, and should resolve #21257 and #21258 once it's tagged.

This unstable sorting was temporarily fixed on version 5.5.5 by #21214, which made sortBy() return stable results...but that PR should remain reverted for now, until a separate issue with that solution has been fixed and tested.

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

Successfully merging this pull request may close these issues.

None yet

4 participants