Skip to content
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

[5.8] Allow belongsToMany to take Model/Pivot class name as a second parameter #27774

Merged
merged 4 commits into from Mar 5, 2019

Conversation

Projects
None yet
5 participants
@linaspasv
Copy link
Contributor

linaspasv commented Mar 4, 2019

Overview

The motivation behind this RP is to allow a non-hardcoded intermediate table name assignments in belongsToMany relationship. The proposed PR does not alter old behavior but improves and provides more integrity for people using Pivot models.

Example

namespace App\Models;

class Customer extends Model
{
    protected $table = 'customers';
}

class CustomerProfile extends Pivot
{
    protected $table = 'customers_profiles';
}

Example of current usage (hardcoded):

return $this->belongsToMany(Profile::class, 'customers_profiles')->using(CustomerProfile::class);

Example of current usage (dynamic):

$pivotClass = CustomerProfile::class; 
return $this->belongsToMany(Profile::class, (new $pivotClass)->getTable())->using($pivotClass);

Example of (possible) usage after PR (improved behavior does not require to define Pivot through using):

return $this->belongsToMany(Profile::class, CustomerProfile::class);

linaspasv added some commits Mar 4, 2019

@mfn

This comment has been minimized.

Copy link
Contributor

mfn commented Mar 4, 2019

I don't understand why. The current code is perfectly fine, isn't it? Only to save a few lines?

@linaspasv

This comment has been minimized.

Copy link
Contributor Author

linaspasv commented Mar 4, 2019

@mfn it's useful for someone who uses Pivot models, especially with custom intermediate table names. As I have noted the old behavior is not changed and there is no breaking change introduced. Pivot class is an extension of a Model class, and until now setting protected $table = '.....' attribute in Pivot models did not have much usage. Finally, the less hardcoded parts we have the more easier is to maintain our applications.

I hope I have cleared your doubts that it's not just to save a few lines (even so Code Less. Do More.)

edit
you could achieve the same behavior with the code below if you would love to approach in a non-hardcoded way but it just does not look right.

$pivotClass = CustomerProfile::class; 
return $this->belongsToMany(Profile::class, (new $pivotClass)->getTable())->using($pivotClass);
return $object->getTable();
}
return $class;

This comment has been minimized.

@staudenmeir

staudenmeir Mar 5, 2019

Contributor

What's a scenario where this line (212) is reached?

This comment has been minimized.

@linaspasv

linaspasv Mar 5, 2019

Author Contributor

Line (212) is reached when a given table name can't be resolved as an instance of a Model class, so in all other cases it just fallback to the old behavior.

This comment has been minimized.

@staudenmeir

staudenmeir Mar 5, 2019

Contributor

So for the rare case that a table name coincidentally is also a valid class name?

This comment has been minimized.

@linaspasv

linaspasv Mar 5, 2019

Author Contributor

My code checks if the given table name is a valid class name but also extends Model / Pivot class. The only rare case I could think of could be when user has a Pivot class named "customers_profiles " without a namespace and uses "customers_profiles" string as a second parameter for belongsToMany:

// note: no namespace used

class customers_profiles extends Pivot
{
    protected $table = 'customers_profiles';
}

......

return $this->belongsToMany(Profile::class, 'customers_profiles')

In this case, my code will see that's the valid Pivot class, will try to load Pivot model and take table name through getTable() method.

dd((new customers_profiles)->getTable()); 
// gives back "customers_profiles" string, 
// or 
// auto-generated value "customers_profile" when user has not defined $table attribute

All other cases I could think of should fallback to an old behavior.

I think the following very rare case should not be an issue. Let me know if I missed something.

Thanks.

This comment has been minimized.

@linaspasv

linaspasv Mar 5, 2019

Author Contributor

p.s. I have slightly adjusted the code flow / naming.

@mfn

This comment has been minimized.

Copy link
Contributor

mfn commented Mar 5, 2019

you could achieve the same behavior with the code below if you would love to approach in a non-hardcoded way but it just does not look right.

Thanks, makes sense to me 👍

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Mar 5, 2019

@linaspasv it would make more chance of being accepted if the PR contains a test as well.

@taylorotwell taylorotwell merged commit e1952ab into laravel:5.8 Mar 5, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.