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

Improve collection stubs #556

Merged
merged 3 commits into from
May 5, 2020
Merged

Improve collection stubs #556

merged 3 commits into from
May 5, 2020

Conversation

Daanra
Copy link
Contributor

@Daanra Daanra commented May 2, 2020

This MR ensures that an EloquentCollection can also contain non-model values.
Keys are still hardcoded as ints this should be changed in the future.

The more I looked into properly type-hinting the collection methods using stubs the more I realised what a can of worms I had opened. The return-types of the collection methods can be really counter-intuitive. Take for example:

EloquentCollection::times(5); // Returns an EloquentCollection

EloquentCollection::times(5, function ($i) {
   return $i;
}); // Returns a SupportCollection

User::all()->mapToGroups(function (User $user) {
   return [$user->email => $user->name];
}) // Returns SupportCollection<string, EloquentCollection<string>>

Another problem with the EloquentCollection methods is that a lot of them use map() internally, and map() may return either a SupportCollection or an EloquentCollection. However, Laravel's PHPDoc states the return type as static which is technically incorrect and may result in false positives.

Further work remains to be done to completely support all the collection magic without any false positives.

Closes #550

@canvural canvural merged commit 0d35a54 into larastan:master May 5, 2020
@nunomaduro
Copy link
Collaborator

@Daanra Just added you to the list of maintainers of this package.

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.

Eloquent Collection map method does not always has model as first parameter of its callback
4 participants