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

[10.x] Support sort option flags on sortByMany Collections #50269

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

TWithers
Copy link
Contributor

The sortBy() method of Collections allows you to pass sorting option flags as the second parameter, which works when the collection is being sorted by a single key. As soon as you want to sort by multiple keys, the sorting option flag is ignored. You can pass in callables instead of collection keys, which will do the job, but it requires you to write several callables.

Here is my real world example:

$this->campaigns = $campaigns
    ->with('folder', 'campaignCategory')
    ->get()
    ->sortBy([
        fn ($a, $b) => str($a->folder?->name)->lower() <=> str($b->folder?->name)->lower(),
        fn ($a, $b) => str($a->campaignCategory->name)->lower() <=> str($b->campaignCategory->name)->lower(),
        fn ($a, $b) => str($a->name)->lower() <=> str($b->name)->lower(),
    ])

I could shorten the code by using strcasecmp() but I feel like just using the sorting flags option would be the simplest. It would allow me to reduce the above code to this:

$this->campaigns = $campaigns
    ->with('folder', 'campaignCategory')
    ->get()
    ->sortBy(['folder.name', 'campaignCategory.name', 'name'], SORT_NATURAL | SORT_FLAG_CASE)

This PR supports all the flags currently supported by the different array sort() functions. I included tests for each specific sorting flag since they all had to be implemented manually as the sortByMany() method relies on usort().

Since the $options flag was never passed to sortByMany(), I didn't see this as a bc issue, so I have targeted the 10.x branch.

@taylorotwell taylorotwell merged commit d7235b3 into laravel:10.x Mar 4, 2024
24 checks passed
@francoism90
Copy link

francoism90 commented Mar 7, 2024

Hmm, this seems to cause some issues for me:

[2024-03-07 20:32:28] local.ERROR: Illuminate\Support\Collection::sortByMany(): Argument #2 ($options) must be of type int, array given, called in /app/vendor/laravel/framework/src/Illuminate/Collections/Collection.php on line 1400 {"userId":1,"exception":"[object] (TypeError(code: 0): Illuminate\\Support\\Collection::sortByMany(): Argument #2 ($options) must be of type int, array given, called in /app/vendor/laravel/framework/src/Illuminate/Collections/Collection.php on line 1400 at /app/vendor/laravel/framework/src/Illuminate/Collections/Collection.php:1434)
[stacktrace]

I use something like this:

return $this->getMedia('clips')->sortByDesc(
    ['custom_properties->bitrate', 'desc'],
    ['custom_properties->height', 'desc'],
    ['custom_properties->width', 'desc'],
);

I think it needs to be defined differently, so I guess an user error?

Edit: it should be defined now as:

return $this->getMedia('clips')->sortByDesc([
    'custom_properties->bitrate',
    'custom_properties->height',
    'custom_properties->width',
], SORT_DESC);

I do like this change! It makes it way more clean and better to work with. :)

@TWithers
Copy link
Contributor Author

TWithers commented Mar 7, 2024

return $this->getMedia('clips')->sortByDesc([
    'custom_properties->bitrate',
    'custom_properties->height',
    'custom_properties->width',
]);

That is just shorthand for this:

return $this->getMedia('clips')->sortBy([
    'custom_properties->bitrate',
    'custom_properties->height',
    'custom_properties->width',
], descending: true);

Which I don't think will actually work since the sortByDesc method doesn't convert the array to a multi-dimensional array, it just passes it through to the sortBy method. And the sortBy method doesn't pass the $descending parameter to the sortByMany method. This could be an additional improvement.

This is the proper, working way to do it:

return $this->getMedia('clips')->sortBy([
    ['custom_properties->bitrate', 'desc'],
    ['custom_properties->height', 'desc'],
    ['custom_properties->width', 'desc'],
]);

@francoism90
Copy link

@TWithers Thanks for providing the proper solution! It seems I just needed to wrap it into an array. :)

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.

5 participants