[8.x] Convert EloquentCollection to base if not solely filled with models

[spawnia]

Some collection methods may introduce non-model items
into an EloquentCollection. This was previously only checked for map().
This PR extends this check to include other such methods.

Another group of methods can be known ahead of time to return a base Collection, in
those cases the check is omitted, the collection is converted toBase()
and the call is passed along.

This achieves a few things:

• Prevent bugs by having more accurate return type hints
• Ensure correct behaviour for code that has special treatment for EloquentCollections
• Aid static analysis tools

[spawnia]

There is another group of methods that might taint an EloquentCollection by adding non-model elements: push(), prepend() and so on. We cannot easily fix those in the same way that map() and others work, because they return $this.

While it would certainly be possible to return a new base collection instead, not mutating the original and not returning the same instance would be quite a big breaking change, so i decided against it.

If we wan't to go further in this direction, we might have to change those methods to throw if non-models are added in. That would be quite a drastic change, considering the value of strongly typed collections in a language without generics is pretty limited anyways.

I think this current approach is a good middleground, shouldn't break existing code, but prevent or fix lingering bugs.

[spawnia]

I think EloquentCollections assume all contained models to be the same class, so we might change this check to the following:

        $model = $maybeEloquent->first() ?? Model::class;

        foreach($maybeEloquent as $maybeModel) {
            if(! $maybeModel instanceof $model) {
                return $maybeEloquent->toBase();
            }
        }

        return $maybeEloquent;

What do you think?

[driesvints]

Ping @JosephSilber

[JosephSilber]

I'm not a fan, and don't think having added it to map in the first place was a good idea.

1. Shoehorning our own runtime type-checks into a language that doesn't even support generics is a losing proposition.

2. This doesn't even fix it. An Eloquent Collection is really meant to contain only a single type of model, otherwise contains, load, modelKeys and many other methods are simply broken.

   So what now? Are we going to compare the class names of the models? Are we going to check their inheritance trees? Maybe use instanceof? What if there's just a common ancestor? What about when people have different (non-inheriting) models for the same table? As I said: a losing proposition.

   If we really need this, we should be petitioning for getting generics into PHP itself.

3. This adds another, full, extra iteration to these methods, for not much of a gain.

I believe we should only return a base collection when we are the ones changing the values to non-models.

In any other case, let the user call toBase() directly. They know more about their code than we ever could.

[spawnia]

@JosephSilber i agree with the point you make. As long as PHP lacks generics, we won't be able to fully guarantee the integrity of the items in an EloquentCollection. The issues you outlined in 2. are especially striking. Safety is great and all, but we certainly have to balance it with performance considerations.

My intent with this PR is not necessarily to push in the direction of adding more validation, but rather to have consistency. I don't see a reason why map() should get special treatment whereas replace() does not. I am also fine with removing the check from map() if there is consensus for it.

In cases where we can know ahead of time the result definitely won't be a collection that holds models, calling toBase() on the users behalf seems reasonable and cheap. Adding the missing methods here just adds consistency.

Nit: There might be rare cases where that is not desirable, e.g.:

Post::all()
    ->groupBy('some_attribute')
    ->filter(function ($group) { return $group->count() > 3; })
    ->flatten()
    ->load('someRelation');

[taylorotwell]

Will hold off on this for now.

[JosephSilber]

@taylorotwell I would even go a step further, and propose that having added that to map is actually harmful, because the return type of the method now changes based on the amount of items in the original collection (this is similar, by not the same, as the problem with random that was fixed in #16865).

Consider the following piece of code:

$inactiveUsernames = User::where('active', false)->get()->map->name;

// Now try using the $inactiveUsernames collection elsewhere

What's the type of $inactiveUsernames? There's no way to know by looking at the code. It wholly depends on whether there are any inactive users in our DB.

• Suuport\Collection - if there were actually inactive users, we'd get a Support collection
• Eloquent\Collection - if there were no inactive users, then the collection doesn't contain any non-model values, so we get an Eloquent collection.

This is not only confusing, but can lead to subtle bugs that are extremely tricky to debug.

---

I vote that we revert map to always return an Eloquent collection.

Let users call toBase() directly whenever they need to.
They know more about their code than we ever could.

[spawnia]

@JosephSilber Agreed. What is your take on the implicit toBase() call in methods such as keys()?

I am starting to think we are better off to stick with an Eloquent\Collection even there, following the principle of least surprise.

We might have to fix methods such as contains() or merge() that assume an Eloquent\Collection always and only holds models. It is simply not realistic that we are able to uphold that guarantee.