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

Collection sorting appears to be altered on v10.1.0 #46213

Closed
browner12 opened this issue Feb 21, 2023 · 3 comments
Closed

Collection sorting appears to be altered on v10.1.0 #46213

browner12 opened this issue Feb 21, 2023 · 3 comments
Assignees

Comments

@browner12
Copy link
Contributor

  • Laravel Version: 10.1.0
  • PHP Version: 8.2.2
  • Database Driver & Version: MySQL 8.0.32

Description:

After upgrading to Laravel 10.1.0, I was getting some test failures that appeared to be related to Collection sorting. I reverted back to Laravel v10.0.3 and all tests passed again.

I haven't had time to deep dive in yet, but will soon and report back with more info.

Steps To Reproduce:

Possibly use the Collection sort() method. Will provide more details as I dig in.

@zeriyoshi
Copy link

Are you using one of the following functions? If so, the #46105 may be affected

  • srand
  • rand
  • mt_srand
  • mt_rand
  • shuffle
  • str_shuffle
  • array_rand

@browner12
Copy link
Contributor Author

Yes, was able to trace the issue back to #46105, and specifically because of my use of the Collection::random() method.

In <10.1.0, calling random() would get random elements, but they would always be returned in the order they existed in the original collection. In >=10.1.0, you now actually get back random elements in a random order.

Before:

$collection = new Collection([
    'alpha',
    'bravo',
    'charile',
]);

dump($collection->random(3)->toArray());

//['alpha', 'bravo', 'charlie'];

After:

$collection = new Collection([
    'alpha',
    'bravo',
    'charile',
]);

dump($collection->random(3)->toArray());

//['alpha', 'charlie', 'bravo'];
//['charlie', 'bravo', 'alpha'];
//['bravo', 'alpha', 'charlie'];

For most of my tests I didn't actually care about the order, so I was able to switch from using assertEquals() to assertEqualsCanonicalizing, which does not require the order of arrays to be the same.

In other tests I had to sort my random elements to make sure they specifically matched returned results.


As minor as this is, it feels like a breaking change to me. Even though Laravel does not explicitly call out the order elements may be returned, because Arr::random() defers to PHP's array_rand(), I feel like we are required to maintain that behavior. The array_rand() documentation explicitly states:

If multiple keys are returned, they will be returned in the order they were present in the original array.

https://www.php.net/manual/en/function.array-rand.php

For me specifically, this just exposed some brittleness/assumptions in my tests, but for other people it may have a more serious impact.

@nunomaduro
Copy link
Member

It should be fixed on the 10.1.2 release. Thank you for reporting this.

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

No branches or pull requests

4 participants