fix attribute vs relation priorty

[mshamaseen]

Attributes should be prioritized over relationship methods; this is how Laravel does it and what the developer anticipates.

The issue

The issue was (and I added a test for it), using this package, if you have a method name same as an attribute name, and you call the attribute, this package will return an error instead of returning the attribute.

for example:

/**
* @property string foo
*/
class model {
  function foo() {
    // this is not a relation, it is an action, do anything here
  }
}

Calling model->foo will throw an exception

The solution

This package was trying to call the method before the attribute because of embedded relations and (strangely) the many-to-many relation.

What I did in this PR is, Instead of calling the methods first, check if there is a method that has a return type of EmbedsOne or EmbedsMany and return it, otherwise continue using Laravel attribute/relation workflow (the same logic Laravel team applied for Attribute accessor functions).

For the BelongsToMany relation, I noticed that in the tests, the foreign key was called the same as the relation, let us say group.

So when $model->group was called, the tests expected the relation instead of the attribute, which is wrong, because attributes should be prioritized over relations.

The solution was so easy, just don't name the foreign key the same as the relation; that is the default behavior if you don't pass any foreign keys to $this->belongsToMany(related , /* Don't pass anything else */) , and that is also the documented way to do it.

Extra

While I was working I updated MySql/MongoDB versions in the dockerfile and github action file. I also dropped the support for mongoDB version 4 as starting from version 5 and above Mongosh is used instead of Mongo.

[GromNaN]

Thank you for contributing your change back to the original project.

I think the changes you've suggested are useful, and will help us better stick to the way Eloquent works.

It needs to be review by @alcaeus and @divine.

[GromNaN]

What I did in this PR is, Instead of calling the methods first, check if there is a method that has a return type of EmbedsOne or EmbedsMany and return it, otherwise continue using Laravel attribute/relation workflow (the same logic Laravel team applied for Attribute accessor functions).

Very smart. Can you please provide a link to where it's done like this in laravel.

[mshamaseen]

hasAttributeMutator method on https://github.com/laravel/framework/blob/7f287ed2ee8aa0067b66c002b582f8ff7eedc00c/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L440C20-L440C39

[divine]

Hello,

Any decision regarding embedded relations was made for v4.0 @GromNaN? I mean will be it rewritten completely or just "fixed" when a problem occurs?

We talked about it a lot and the way it currently "works" can lead to performance and logical problems.

My suggestion is that there should be a final decision made before v4.0 otherwise it can lead to Achilles's heel.

Thanks!

[GromNaN]

Hello,

Any decision regarding embedded relations was made for v4.0 @GromNaN? I mean will be it rewritten completely or just "fixed" when a problem occurs?

We talked about it a lot and the way it currently "works" can lead to performance and logical problems.

My suggestion is that there should be a final decision made before v4.0 otherwise it can lead to Achilles's heel.

Thanks!

To meet the need for a rapid release of a Laravel 10-compatible version, we won't be reworking the relationships for the time being.

[mshamaseen]

@GromNaN any news about when this issue will be solved? your work is much appreciated!