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

[6.x] Turn the eloquent collection into a base collection if mapWithKeys loses models #35129

Merged
merged 2 commits into from Nov 9, 2020
Merged

[6.x] Turn the eloquent collection into a base collection if mapWithKeys loses models #35129

merged 2 commits into from Nov 9, 2020

Conversation

nuernbergerA
Copy link
Contributor

@nuernbergerA nuernbergerA commented Nov 6, 2020

Hey guys.

If you map over an eloquent collection and return an array instead of a model you will receive a support collection.

But if you use mapWithKeys it will lead to errors.

If you try to serialize, it will explode in the sky because of this check.

I noticed map has a custom implementation on the eloquent collection to I added one for mapWithKeys with the same behavior.

I didn't extract the code to check and decide which collection type to return, to keep this PR light.

Also noticed there is a mapToDictionary, should we handle this too?

Have a nice day
Adrian

@driesvints
Copy link
Member

Ping @JosephSilber

@JosephSilber
Copy link
Member

JosephSilber commented Nov 9, 2020

  1. I was never a fan of returning a support collection from map. It leads to unexpected results (this just being one of many) and is just a losing proposition. We'll be chasing our tail forever on this futile endeavor.

  2. That said, if we still decide for some reason to keep doing this dance, then of course mapWithKeys needs to work the same way map does.

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.

None yet

4 participants