-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
BelongsTo Relations break when using custom collections #53241
Comments
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.
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! |
Make sure your |
I was using an |
Well, the collection returned from an Eloquent model should be an Eloquent Collection. It assumes some specific methods for relation matching, and other operations. |
Would it be possible for Laravel to then expect an Eloquent Collection Contract instead of an instance of Eloquent Collection |
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
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 How would the 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 |
More concretely, can you share some of the code where you stumbled upon that error message? |
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 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 |
There no benefit of LazyCollection vs Collection here as Models will already been hydrated from database. |
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.
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 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. |
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 aTypeError
because\Illuminate\Database\Eloquent\Relations\BelongsTo::match()
expects an instance anIlluminate\Database\Eloquent\Collection
instead of a custom collectionMy suggestion is to introduce a
Collection
contract whichIlluminate\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
On another model reference that model as the
belongsTo
relationExpected behaviour
Just works
Actual Behavior
The text was updated successfully, but these errors were encountered: