laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.55k stars 11.03k forks source link

BelongsTo Relations break when using custom collections #53241

Closed SlimGee closed 3 weeks ago

SlimGee commented 3 weeks ago

Laravel Version

11.28.1

PHP Version

8.3.12

Database Driver & Version

MariaDB 11.5.2

Description

When using a custom collection on an eloquent model as suggested in the docs here, if that model is referenced as a belongsTo by other other models, it throws a TypeError because \Illuminate\Database\Eloquent\Relations\BelongsTo::match() expects an instance an Illuminate\Database\Eloquent\Collection instead of a custom collection

My suggestion is to introduce a Collection contract which Illuminate\Support\Collection must implement and that is used as the expected types on all other internal models instead of expecting a specific implementation. I think this would be best since collections are a big part of the Laravel framework.

Steps To Reproduce

Tell eloquent to use a custom collection on a model

class User extends Model
{
    /**
     * Create a new Eloquent Collection instance.
     *
     * @param  array<int, \Illuminate\Database\Eloquent\Model>  $models
     * @return \Illuminate\Database\Eloquent\Collection<int, \Illuminate\Database\Eloquent\Model>
     */
    public function newCollection(array $models = []): Collection
    {
        return new UserCollection($models);
    }
}

On another model reference that model as the belongsTo relation

class Profile extends Model
{
  public function user()
  {
   return $this->belongsTo(User::class);
  }
}

Expected behaviour

Just works

Actual Behavior

TypeError: Illuminate\Database\Eloquent\Relations\BelongsTo::match(): Argument #2 ($results) must be of type Illuminate\Database\Eloquent\Collection, Illuminate\Support\LazyCollection given, called in...
crynobone commented 3 weeks ago

Hey there, thanks for reporting this issue.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here?

Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

rodrigopedra commented 3 weeks ago

Make sure your UserCollection extends Illuminate\Database\Eloquent\Collection.

SlimGee commented 3 weeks ago

I was using an Illuminate\Support\LazyCollection

rodrigopedra commented 3 weeks ago

Well, the collection returned from an Eloquent model should be an Eloquent Collection.

It assumes some specific methods for relation matching, and other operations.

SlimGee commented 3 weeks ago

Would it be possible for Laravel to then expect an Eloquent Collection Contract instead of an instance of Eloquent Collection

rodrigopedra commented 3 weeks ago

You can try a PR. But I think it is very unlikely. Unless you provide clear use-cases, and benefits for maintaining the new class inheritance model.

Actually, I don't see much of a benefit. There would be many methods to reimplement yourself. And what would be the disadvantage of just extending Illuminate\Database\Eloquent\Collection?

LazyCollections are meant to work with generator data sources.

To supplement the already powerful Collection class, the LazyCollection class leverages PHP's generators to allow you to work with very large datasets while keeping memory usage low.

Reference: https://laravel.com/docs/11.x/collections#lazy-collection-introduction

In contrast, an Eloquent Collection is usually always hydrated from the Eloquent's Query Builder from a database query's results.

I don't get the need to return a LazyCollection, as Eloquent hydrates the models itself upon database retrieval. How would a generator data source fit in Eloquent's dynamics?

How would the Relation subclasses are supposed to work in that scenario? As when matching the results, they are building the collection piece by piece?

If you can share your use case, you've got me curious about it.

Alternatively, you can try sending a PR with an Eloquent's LazyCollection, one that extends from Illuminate\Database\Eloquent\Collection.

rodrigopedra commented 3 weeks ago

More concretely, can you share some of the code where you stumbled upon that error message?

SlimGee commented 3 weeks ago

I don't "really" need to use a LazyCollection, but the idea of generators being able to keep memory low got me interested, I wanted to see if I can instead have a LazyCollection for all models since the docs suggest using a custom collection.

My Code looked like this

class User extends Model
{
    public function newCollection(array $models = [])
    {
        return LazyCollection::make($models);
    }
}

From that the thrown exception I looked into the source and Eloquent is expecting specifically an Eloquent Collection which seem strange because almost everything else in Laravel is based on interfaces. If I wanted to have my custom collection I should implement a Illuminate\Contracts\Database\Eloquent\Collection and choose to extend Illuminate\Database\Eloquent\Collection if I want or have a different implementation of my own in this it will be extending LazyCollection then implementing Eloquent specific methods which will be enforced by the contract. I don't know if it makes sense.

crynobone commented 3 weeks ago

There no benefit of LazyCollection vs Collection here as Models will already been hydrated from database.

rodrigopedra commented 3 weeks ago

but the idea of generators being able to keep memory low got me interested

Well, they will only keep memory low if being used, as @crynobone said, that is not the case with Eloquent as it hydrates the models from the database.

You'd need to change of Eloquent's own code to make sure it instantiates everything with generators, instead of hydrating the way it does. Otherwise, there would be no benefit on using it.

almost everything else in Laravel is based on interfaces

You're right, almost. Almost is the key here.

In components where the ability to swap moving parts is desired, the code to a contract/interface is there to help this out. That is the case in everything Laravel uses underlying "drivers", such as the queue, and the file system components, just to name a few.

On components, such as Eloquent, where tight coupling is desired, as it has many moving parts that should work together, and each part has strong expectations over the other, coding to a contract is not available.

Take relations for example. I had the need, on a past project, to write a custom relation class to overcome a cumbersome inherited DB structure we could not change.

There are no contracts/interfaces, there is an abstract Relation base class, and of course, you can extend any other built-in relation classes if you see fit.

Rewriting just the methods we needed for our custom relation was already a very hard task, as many methods have tight expectations on how other methods should behave. I can't even imagine if we needed to code every single method against an interface to satisfy everything that Eloquent needs.