[5.5] Belongs to many related local key

[alberto-bottarini]

Hi, I've added also the configurability of the "related" local key using BelongsToMany relationship.

Take this example

USERS
- id
- email

ROLES
- id
- code

ROLE_USER
- user_email
- role_code

With this PR we can define this relationship like this:

class User
{
    public function roles() 
    {
         return $this->belongsToMany('App/Role', 'role_user', 'user_email', 'role_code', 'email', 'code');
    }
}

class Role
{
    public function users() 
    {
         return $this->belongsToMany('App/User', 'role_user', 'role_code', 'user_email', 'code', 'email');
    }
}

PS sorry for this second PR, I should have done one.

[taylorotwell]

In this really needed. It's starting to feel silly. Passing 6-7 arguments to belongs to many?

[alberto-bottarini]

There are a lot of needed and potentially useful options. It's a compromise, most of them are optional arguments. Why HasOne, HasMany and BelongsTo have all the available options out-of-the-box and BelongsToMany not?

Working with legacy dbs, I had a lot of issue about this topic.
The only way to use BelongsToMany, without extend and override it, was to change at runtime the models' primary keys, but I consider this an anti-pattern.

What do you think?

[riesjart]

One suggestion would be to keep the belongsToMany method in the HasRelationships trait the same and use setters for both keys instead. The setters can return '$this'. This way the parameter count remains the same, but one can adjust the relation for that one particular case.

[KKSzymanowski]

It would be nice to have a fluent interface for setting up relationships.
Post-construct setters won't work because eg. joins are set up during object instantiation and they rely on these parameters, but how about:

return $this->belongsToMany('App/Role', function ($relationship) {
    $relationship->setTable('role_user')
                 ->setForeignKey('user_email')
                 ->setRelatedKey('role_code')
                 ->setParentKey('email')
                 ->setLocalKey('code')
                 ->setRelationName('foo');
});

This way you wouldn't have to remember the order of arguments and also if you just want to change eg. the relation name you wouldn't have to pass the silly null, null, null, null, null in the belongsToMany call.

[KKSzymanowski]

There is at least one problem with implementing this solution.

set* methods need to be public in order to use them in a closure but exposing them would confuse users because they might think they can eg. set the table name after the relationship has been instantiated.

We could always bind the closure to the relationship object(and use protected methods) but this would be probably even more confusing to use $this meaning BelongsToMany::$this not Model::$this.

One way to solve this is to introduce an intermediate object accepting all needed parameters and then using it to copy the values to the relationship object in the constructor.
Here's an example implementation: https://gist.github.com/KKSzymanowski/512261a9afea64e0c0ffeb1b69915514
The Setter(or whatever you wanna call it) class could check the debug_backtrace()[2]['class'] to provide more meaningful error when an inappropriate method is called but I don't know how reliable would that be so I didn't include that.
Provided implementation is about 20x slower then explicitly passing all arguments(tested for 7), but it's still quite fast(1 000 000 instantiations take 4 seconds) and most of the time the default values are good enough so this will be used only in edge cases.
EDIT: Without reflections it will probably be a lot faster, haven't tested it though.

Or, you know, the Model::belongsToMany could accept an array of options merged with the defaults, but it's 2017, we don't do that anymore.

[lkmadushan]

@KKSzymanowski nice idea.

[arjasco]

@KKSzymanowski I was definitely going to have a go with this idea myself. 😄 Makes much more sense to me, I do sometimes forget the order of the foreign key and local key. With your example it makes it very obvious what all the columns are.

Not sure on the closure business, but I'm think this could be adapted to allow both ways. I would lose the set bit too. Just..

$this->belongsToMany(Role::class)->foreignKey(...)->relatedKey(...)

👍

[alberto-bottarini]

The idea is cool, more fluent and clear (clearer than the list of parameters).
But for consistency, this approach should be adapted for all the optional parameters in all the relation type object (BelongsTo, HasMany...).

@arjasco your example collides with the confusion issue raised by @KKSzymanowski: the new methods you suggest, will be available also after the relationship definition.

[KKSzymanowski]

@taylorotwell Are you interested in my proposal? I can have it done by the weekend for all relationships.

[oceanwap]

Things get complicated sometimes, might not be useful for everyday but optional configuration is a must thing.

[taylorotwell]

@KKSzymanowski i think it looks pretty good. I would avoid "set" on the front of all the methods and just have it be like ->table ->foreignKey, etc.

[KKSzymanowski]

@taylorotwell Do you want it for 5.4 or 5.5?