Framework: key inconsistency between base & eloquent collections

[leewillis77]

• Laravel Version: 5.7.13
• PHP Version: 7.1.24
• Database Driver & Version: MySQL

Description:

The documentation for eloquent collections (https://laravel.com/docs/5.7/eloquent-collections#available-methods) states that

All Eloquent collections extend the base Laravel collection object; therefore, they inherit all of the powerful methods provided by the base collection class

However, there are cases where the two function differently and this can lead to subtle bugs in code built on the framework. Particularly the two collection types differ in how keys are treated with some Eloquent collection methods ignoring specifically assigned keys of the collection in favour of a key structure based on $model->getKey().

TL;DR

You have a situation where

Model::all()->keyBy('property')->get('property_value')

returns a value, yet

Model::all()->keyBy('property')->only(['property_value'])

returns nothing.

Steps To Reproduce:

Take a standard collection of two objects, each with two properties, e.g.

$collection = collect([
    ['name' => 'foo', 'product_code' => '123'],
    ['name' => 'bar', 'product_code' => '456']
]);

You can key this collection by product code, and filter it to only a specific name using the keyBy and only collection methods, e.g.

$collection->keyBy('name')->only(['foo']);

This returns a filtered collection containing only the item keyed by the value "foo":

=> Illuminate\Support\Collection {#2897
     all: [
       "foo" => [
         "name" => "foo",
         "product_code" => "123",
       ],
     ],
   }
>>>

Now imagine that you have a model (we'll call it Product for sake of argument) with the same two properties, name and product_code. If you pull a collection of those from the database and try and do the same thing you'll hit problems.

Product::all()->keyBy('name');

Returns as you'd expect:

=> Illuminate\Database\Eloquent\Collection {#2892
     all: [
       "foo" => App\Product {#2886
         id: 1,
         name: "foo",
         product_code: "123",
       },
       "bar" => App\Product {#2905
         id: 2,
         name: "bar",
         product_code: "456",
       },
     ],
   }

You can use some key-related collection methods on this collection, such as get():

>>> Product::all()->keyBy('name')->get('foo');
=> App\Product {#2891
     id: 1,
     name: "foo",
     product_code: "123",
   }

However, only, except do not work correctly with this keyed collection, so with the same collection as the previous example, only fails to find the keyed item:

>>> Product::all()->keyBy('name')->only(['foo']);
=> Illuminate\Database\Eloquent\Collection {#2904
     all: [],
   }

except fails to exclude it:

>>> Product::all()->keyBy('name')->except(['foo']);
=> Illuminate\Database\Eloquent\Collection {#2889
     all: [
       App\Product {#2908
         id: 1,
         name: "foo",
         product_code: "123",
       },
       App\Product {#2886
         id: 2,
         name: "bar",
         product_code: "456",
       },
     ],
   }

So - we're in the situation where get($key) returns an item, but only([$key]) does not.

The root cause seems to be the Eloquent collection's "only" and "except" methods (and others) which insist on running on a separately built "dictionary" (https://github.com/laravel/framework/blob/5.7/src/Illuminate/Database/Eloquent/Collection.php#L364) keyed by the model's getKey() results (https://github.com/laravel/framework/blob/5.7/src/Illuminate/Database/Eloquent/Collection.php#L417) and ignore any keys that have been set.

I assume that this is there so that you can run key-based sensibly on collections pulled straight from the database (which are un-keyed) - however I think it certainly needs to take into account where the collection has been specifically keyed somehow.

Reading through the Eloquent Collection class it seems that getDictionary() is used in many of the methods so I'd expect similar misleading results from some of those, although I think careful thought needs to be taken over how things are changed to avoid introducing similar unexpected behaviour.

Note: This was previously reported in #16976 but I don't think it was understood or explained very well there so got closed with no resolution.

[mfn]

Tricky issue.

What could a non-backwards-breaking fix look like?

[driesvints]

Thanks for bringing this to our attention. Imo this should indeed be solved but the question is how.

[leewillis77]

It would be interesting to see if the collection can understand whether it's been keyed (maybe by having the relevant methods toggle an internal flag or similar) or not, so it can use the keys if they exist, or fall back on the $model->getKey() behaviour.

[samrap]

This would be backwards-incompatible but it feels like if any operation is performed on an Eloquent Collection that causes it to change (i.e. keyBy), it is no longer an Eloquent Collection and should return a new instance of Illuminate\Support\Collection. IMO an Eloquent Collection represents a result returned from the database, if you modify the structure of that collection it is no longer representative of the query result.

[taylorotwell]

->toBase()->only()