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 sortBy does not work if collection elements are stdClass #35947

Closed
tm1000 opened this issue Jan 19, 2021 · 5 comments · Fixed by #35950
Closed

Collection sortBy does not work if collection elements are stdClass #35947

tm1000 opened this issue Jan 19, 2021 · 5 comments · Fixed by #35950

Comments

@tm1000
Copy link
Contributor

tm1000 commented Jan 19, 2021

  • Laravel Version: 8.23.1
  • PHP Version: 7.4.13
  • Database Driver & Version: 10.5.8-MariaDB-1:10.5.8+maria~focal

Description:

When running raw database queries through illuminate the returned response is a collection of stdClass objects (This is because the default fetch mode for PDO is set to PDO::FETCH_OBJ). I'm not sure if this is the intended result but these objects can't be sorted because stdClass does not implement the ArrayAccess interface. Thus when Laravel Collections tries to sort the collection all results end up returning null and it's unsortable.

I would say this is a non issue at first but because any queries return a collection one would think it should work. I assume other methods from collections will have issues from any raw results as well.

Steps To Reproduce:

$a = new stdClass();
$a->name = 'a';
$b = new stdClass();
$b->name = 'b';
$c = new stdClass();
$c->name = 'c';

$collect = collect([$b, $a, $c]);

$collect = $collect->sortBy([['name', 'asc']]);

Output:

array (
  0 => 
  (object) array(
     'name' => 'b',
  ),
  1 => 
  (object) array(
     'name' => 'a',
  ),
  2 => 
  (object) array(
     'name' => 'c',
  ),
)

A "quick" fix online is to just cast all stdClass to array through map then sort:

$collect = $collect->map(function ($x) {
    return (array) $x;
});

$collect = $collect->sortBy([['name', 'asc']]);

Output:

array (
  0 => 
  array(
     'name' => 'a',
  ),
  1 => 
  array(
     'name' => 'b',
  ),
  2 => 
  array(
     'name' => 'c',
  ),
)

This does indeed correct the issue but it seems to over complicate something that I think collections could detect. That being the usage of stdClass which can be temporarily converted to perform the desired functions.

@derekmd
Copy link
Contributor

derekmd commented Jan 19, 2021

Your example Collection@sortBy() call will need to follow the documentation's example: https://laravel.com/docs/8.x/collections#method-sortby The query Builder@orderBy() method allows defining the sort direction parameter for values 'asc' or 'desc' but Laravel's collection sort methods don't support such a parameter.

Instead call $collect->sortBy('name') (ascending) or $collect->sortByDesc('name'):

$a = new stdClass;
$a->name = 'a';
$b = new stdClass;
$b->name = 'b';
$c = new stdClass;
$c->name = 'c';

collect([$b, $a, $c])->sortBy('name');
// => Illuminate\Support\Collection {
//   all: [
//     1 => {
//       "name": "a",
//     },
//     0 => {
//       "name": "b",
//     },
//     2 => {
//       "name": "c",
//     },
//   ],
// }

collect([$b, $a, $c])->sortByDesc('name');
// => Illuminate\Support\Collection {
//   all: [
//     2 => {#3419
//       "name": "c",
//     },
//     0 => {
//       "name": "b",
//     },
//     1 => {
//       "name": "a",
//     },
//   ],
// }

If you have a look at the framework's call stack for Collection@sortBy(), you can see it does support stdClass properties through the data_get() global helper function:

  • Illuminate\Support\Collection@sortBy()
  • calls Illuminate\Support\Traits\EnumeratesValues@valueRetriever()
  • which calls data_get() in file Illuminate/Support/helpers.php
$c = new stdClass;
$c->name = 'c';
data_get($c, 'name');
// => "c"

@tm1000
Copy link
Contributor Author

tm1000 commented Jan 19, 2021

@derekmd scroll down a bit further in the documentation you linked and you will see that what I said should work the way I proposed according to the 8.x documentation.

This was added in 8.x it appears as this section does not appear in 7.x or lower

It comes under the paragraph that starts with:

If you would like to sort your collection by multiple attributes

Here's a screenshot from the link you sent me. I highlighted the bit that shows it supports the wordage of 'asc' and 'desc'

image

This functionality was added by Taylor in 53eb307

Considering the sortByMany function was taken directly from Arr I'd say it was written in a way that only supports arrays. While instead it should be supporting data_get. I don't see it using data_get at all here

tm1000 added a commit to tm1000/framework-1 that referenced this issue Jan 20, 2021
This fixes laravel#35947. Since collections can deal with objects then the method sortByMany taken from Arr should be able to deal with objects as well
@tm1000
Copy link
Contributor Author

tm1000 commented Jan 20, 2021

I added a fix in the linked PR against the function sortByMany which exists in Collection to support using data_get over Arr::get

@derekmd
Copy link
Contributor

derekmd commented Jan 20, 2021

This was added in 8.x it appears as this section does not appear in 7.x or lower

Ah sorry, I was mistakenly looking at a stale 7.x branch on my local machine and I didn't realize multi-sort was added by 8.x.

The original sortByMany() change was submitted on the Arr class so it looks like it was never fully adjusted when the method was moved to Collection. Your data_get() pull request will fix it but some tests should probably be added to ensure the feature doesn't get broken in the future.

This (less pretty) arrow function syntax also works to handle stdClass multi-sorts:

collect([$b, $a, $c])->sortBy([
    fn($a, $b) => strcmp($a->name, $b->name), // name asc
    fn($a, $b) => $b->id - $a->id, // id desc
]);

@tm1000
Copy link
Contributor Author

tm1000 commented Jan 20, 2021

Thanks @derekmd. I thought about doing that as well. However I'm being told by @GrahamCampbell that Collections are intended to wrap arrays, not objects so at this point I really don't know what the answer is. If anything the documentation should state something to the effect that Objects are not supported in Collections (even though Illuminate returns a Collection of stdClass for results that aren't eloquent because of the default usage of PDO::FETCH_OBJ)

Another solution for me would be to just use Eloquent in this instance

🤷‍♂️

taylorotwell pushed a commit that referenced this issue Jan 20, 2021
This fixes #35947. Since collections can deal with objects then the method sortByMany taken from Arr should be able to deal with objects as well
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 a pull request may close this issue.

2 participants