Skip to content

[12.x] force Eloquent\Collection::partition to return a base Collection #53304

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

Merged

Conversation

browner12
Copy link
Contributor

the way partition currently works is it returns an Eloquent\Collection of Eloquent\Collections, which is technically not allowed by the generic type hints defined by the class, and also doesn't really make any logical sense for how someone would want to use this.

It also causes issues in the IDE because based on the type hints it assumes ALL values of an Eloquent\Collection are of type Eloquent\Model, which is not the case for the partition method.

what we really want is a Support\Collection of Eloquent\Collections. we can accomplish this by first calling the parent::partition method to get our (illegal) result of Eloquent\Collection<int, Eloquent\Collection>, and then converting it to a base collection to end up with the desired Support\Collection<int, Eloquent\Collection>.

//get users
$users = User::all();

//partition by age
$partition = $users->partition(fn ($user) => $user->age > 18);

//before change
dump($partition::class);    // Illuminate\Database\Eloquent\Collection
dump($partition[0]::class); // Illuminate\Database\Eloquent\Collection
dump($partition[1]::class); // Illuminate\Database\Eloquent\Collection

//after change
dump($partition::class);    // Illuminate\Support\Collection
dump($partition[0]::class); // Illuminate\Database\Eloquent\Collection
dump($partition[1]::class); // Illuminate\Database\Eloquent\Collection

someone please double check my generic return type docblock. I'm relatively new to them, but I think this should be correct and fix the issues.

#53298 #53283

the way `partition` currently works is it returns an `Eloquent\Collection` of `Eloquent\Collection`s, which is technically not allowed by the generic type hints defined by the class, and also doesn't really make any logical sense for how someone would want to use this.

It also causes issues in the IDE because based on the type hints it assumes ALL values of an `Eloquent\Collection` are of type `Eloquent\Model`, which is not the case for the `partition` method.

what we really want is a `Support\Collection` of `Eloquent\Collection`s. we can accomplish this by first calling the `parent::partition` method to get our (illegal) result of `Eloquent\Collection<int, Eloquent\Collection>`, and then converting it to a base collection to end up with the desired `Support\Collection<int, Eloquent\Collection>`.
@fragkp
Copy link
Contributor

fragkp commented Oct 25, 2024

and also doesn't really make any logical sense for how someone would want to use this

I'm not sure about that. See this example:

[$adults, $kids] = User::all()->partition(fn ($user) => $user->age > 18);

$adults->update(...);
$kids->delete();

This will only work when the underlying collection is a EloquentCollection.

@browner12
Copy link
Contributor Author

correct, and that's how this change works. because you're destructuring you're not assigning the result of the partition method, you're defining based on the 0 and 1 keys of the return value. so both $adults and $kids are still both Illuminate\Database\Eloquent\Collection, which is what we want, and allow you to call your update() and delete() methods.

@fragkp
Copy link
Contributor

fragkp commented Oct 25, 2024

@browner12 Ah, your right! Good change then!

@taylorotwell taylorotwell merged commit 4d18e66 into laravel:master Oct 29, 2024
33 checks passed
@browner12 browner12 deleted the AB-eloquent-collection-partition branch October 29, 2024 17:49
browner12 added a commit to browner12/docs that referenced this pull request Oct 29, 2024
browner12 added a commit to browner12/docs that referenced this pull request Apr 23, 2025
taylorotwell added a commit to laravel/docs that referenced this pull request Apr 23, 2025
)

* document change in Eloquent Collection partition behavior

document the code change in laravel/framework#53304

* Update collections.md

* Update eloquent-collections.md

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
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.

3 participants