Collection::groupBy should not return a new static() because that might be an EloquentCollection

[rudiedirkx]

Laravel Version

11.23.5

PHP Version

8.2.14

Description

Currently, a collection groupBy() returns the same type of collection, containing the same type of collection: @return static<array-key, static<array-key, TValue>>. That's good for the base collection, of children that accept any type of TValue, but not for EloquentCollection, because that doesn't accept any type of TValue, but only Model: @template TModel of \Illuminate\Database\Eloquent\Model. So groupBy() should check for that (or EloquentCollection could overwrite groupBy()) to not return new static, but new Illuminate\Support\Collection.

Steps To Reproduce

$users = User::all();
assert($users instanceof EloquentCollection); // 🟩 

$groups = $users->groupBy('status');
assert($users instanceof EloquentCollection); // 🟥 because EloquentCollection should only contain Model, not EloquentCollection
assert($users->first() instanceof EloquentCollection); // 🟩 

Fixing might be super backward incompatible, but it is correcter. Workaround is pretty simple though: add in custom model collection (which everyone has, right?):

/** @return BaseCollection<array-key, $this> */
public function groupBy($groupBy, $preserveKeys = false) {
	$groups = parent::groupBy($groupBy, $preserveKeys); // temporary 'impossible' EloquentCollection of EloquentCollections
	return BaseCollection::make($groups->all());
}

[browner12]

I wonder if this is related to my issue #53283

[rudiedirkx]

I think it is, because partition() also returns a static of static, which makes a ModelCollection of ModelCollection, which is illegal. But partition() is even sillier IMO, because why does it even return a collection? It's always only keyed 0 and 1. That could have been an array. But they can't change that now 😆

[cosmastech]

Funny, I came across a similar thing earlier when I was trying to write a docblock for the output. Larastan also apparently does some magic with trying to infer the type. larastan/larastan#1860

In my opinion, there's nothing that stops an EloquentCollection from containing things other than Models (other than good sense). It seems a bit inconvenient to convert this to a base Collection for a case like this.

// Get all orders that have shipped
$orders = Order::where('status', 'shipped')->get();

// Group them by the shipper
$shipperToOrdersCollection = $orders->groupBy('shipper');

foreach($shipperToOrdersCollection as $shipperName => $ordersCollection) {
    // Check the status of the shipment
    $shippingStatus = ShipperApi::for($shipperName)->getShippingStatus();
    // Update all of the orders with the latest status
    $ordersCollection->toQuery()->update(['status' => $shippingStatus]);
}

[rudiedirkx]

In my opinion, there's nothing that stops an EloquentCollection from containing things other than Models (other than good sense).

Except Laravel's own phpdoc that says they must be Models. If Laravel supports phpdoc and templates and static analysis, it shouldn't 'lie' about it.

But I just have ModelCollection of ModelCollection in my app too, because reality <> stan.

[browner12]

just so we're all on the same page, are you referencing this line?

https://github.com/laravel/framework/blob/11.x/src/Illuminate/Database/Eloquent/Collection.php#L15

[rudiedirkx]

Yes, that's the one.

[crynobone]

groupBy() change the key from model IDs to custom string, this make it an invalid format to ModelCollection and would failed when trying to execute certain methods

[rudiedirkx]

Are you arguing for my point ("this make it an invalid format to ModelCollection") or against ("would failed when trying to execute certain methods")? Both are true. I don't see why you would call a ModelCollection method on a group of ModelCollections though: $users->groupBy('status')->load('organization') etc make no sense.

[browner12]

Opened this PR (#53304) to hopefully solve my partition issue. assuming you could do something similar for groupBy()?

[rudiedirkx]

I assume it's too BC breaking. Especially for groupBy.