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

[8.x] Preserve eloquent collection type after calling ->fresh() #34848

Merged

Conversation

calebporzio
Copy link
Contributor

@calebporzio calebporzio commented Oct 15, 2020

This PR fixes unexpected behavior of an Eloquent collection's ->fresh() method.

Current Behavior:

$users = User::all();
$users->fresh(); // Returns instance of "Illuminate\Database\Eloquent\Collection"

$users = User::all()
$users->first()->delete();
$users->fresh(); //  // Returns instance of "Illuminate\Support\Collection"

New Behavior:

$users = User::all();
$users->fresh(); // Returns instance of "Illuminate\Database\Eloquent\Collection"

$users = User::all()
$users->first()->delete();
$users->fresh(); //  // Returns instance of "Illuminate\Database\Eloquent\Collection" as well

Cause of bug:
Calling ->fresh() will not remove non-existent models, it will set them to null.

Now that the collection contains null values, ->fresh()'s internal ->map call will detect that the collection contains non-model values and return a "Illuminate\Support\Collection" instead.

Current Proposed Solution:

  • Wrap the ->map call in a new static to ensure that the class matches the original eloquent collection type (will even support custom eloquent collections this way).

Ideal Solution: (But potentially breaking)

  • When calling ->fresh() on a model collection, REMOVE the items that are not in the database completely (instead of replacing them with null values)

Why I Care:
Livewire supports setting public properties as eloquent collections.

If a developer deletes one of the items in the collection and then calls ->fresh(), because the collection will now be a normal collection and not a model one, Livewire will not recognize it and will break.

I'm happy to PR the "Ideal" solution if it can be merged to 8.x.

Thanks!

@calebporzio calebporzio changed the title [8.x] Preserve eloquent collection class type after calling ->fresh() [8.x] Preserve eloquent collection type after calling ->fresh() Oct 15, 2020
@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Oct 15, 2020

Why don't we just fix map?

@calebporzio
Copy link
Contributor Author

@calebporzio calebporzio commented Oct 15, 2020

Ok, two points:

A) ->map in an eloquent collection isn't broken. It returns an instance of a model collection if all items are models, otherwise, it returns a normal collection.

B) The bug is actually more nuanced than I originally thought (will update the description to reflect shortly):

User::all()->fresh() will ALWAYS return a model collection.

However, if a model inside a model collection doesn't exist in the database, ->fresh() will NOT return a model collection.

Example:

$models = User::all();
$models->add(new User); // Or $models->first()->delete();
$models->fresh(); // Does NOT return a model collection

The reason is because of this line in the ->fresh() definition:

return $this->map(function ($model) use ($freshModels) {
    return $model->exists && isset($freshModels[$model->getKey()])
        ? $freshModels[$model->getKey()] : null;
});

Notice that if any model in the collection does not exist in the database, the map will return null.

Because the collection now contains a null item, the ->map function will notice that not ALL items are a model (some are null), and it will cast to an "Illuminate\Support\Collection"

So ultimately the question is: How do you expect ->fresh() to work.

My expectation would be that it would strip out models that don't exist and still return a model collection.

If we consider filtering out nulls from the result of ->fresh a breaking change, I would say, merge this PR, then write a new one for the master branch that filters out nulls. Happy to do the latter as well.

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 15, 2020

Ping @JosephSilber ... curious your thoughts.

@GrahamCampbell GrahamCampbell marked this pull request as draft Oct 15, 2020
@JosephSilber
Copy link
Member

@JosephSilber JosephSilber commented Oct 15, 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 a losing proposition.

  2. Having fresh return a support collection is definitely wrong.

  3. I much prefer removing models that no longer exist, and I wouldn't consider it a breaking change. Having nulls is totally useless and probably not what anyone wants.

@taylorotwell taylorotwell marked this pull request as ready for review Oct 15, 2020
@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 15, 2020

Thanks for the feedback @JosephSilber

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 16, 2020

@calebporzio I kinda think we should just remove the null values. Not sure it makes sense to keep them really.

@taylorotwell taylorotwell merged commit 914a203 into laravel:8.x Oct 16, 2020
7 checks passed
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