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

[8.x] Sort collections by key when first element of sort operation is string (even if callable) #40212

Merged
merged 4 commits into from
Dec 30, 2021

Conversation

cheryllium
Copy link
Contributor

@cheryllium cheryllium commented Dec 30, 2021

This PR addresses #40203.

The documentation for sortBy() describes: "Each sort operation should be an array consisting of the attribute that you wish to sort by and the direction of the desired sort. [...] When sorting a collection by multiple attributes, you may also provide closures that define each sort operation."

The following example summarizes the issue:

$collection = collect([
    ['name' => 'Taylor Otwell', 'sort' => 34],
    ['name' => 'Abigail Otwell', 'sort' => 30],
    ['name' => 'Taylor Otwell', 'sort' => 36],
    ['name' => 'Abigail Otwell', 'sort' => 32],
]);

$sorted = $collection->sortBy([
    ['name', 'asc'],
    ['sort', 'desc'],
]);

The developer expects the array to be sorted by the "name" and "sort" keys. However, because "sort" is also the name of an existing function (in this case a built-in function), it is considered callable and the framework attempts to call sort() instead, which can throw an error (that sort() was called with invalid parameters)

The change in this PR makes it check if the first element in each sort operation is a string in addition to callable. If it's a string, it will continue to be treated as a key even if it is also the name of an existing function. This PR preserves the ability to pass in a string key or a closure (which would not be a string).

This PR may break functionality that relies on being able to pass in a function name as a string instead of a closure. I think that might be okay since using it that way is undocumented (as far as I could tell?)

I'm new to this so please kindly let me know if I should do anything differently here, or if I'm mistaken about anything. Thank you :)

@cheryllium cheryllium changed the title Sort collections by key when given a string even when key is callable Sort collections by key when first element of sort operation is string (even if callable) Dec 30, 2021
@taylorotwell taylorotwell merged commit 2f86d4e into laravel:8.x Dec 30, 2021
@GrahamCampbell GrahamCampbell changed the title Sort collections by key when first element of sort operation is string (even if callable) [8.x] Sort collections by key when first element of sort operation is string (even if callable) Dec 31, 2021
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

2 participants