-
Notifications
You must be signed in to change notification settings - Fork 271
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
use table name for belongsToMany() #643
use table name for belongsToMany() #643
Conversation
I don't think it's true that it always needs a name. Only when it's unconventional. In addition, all tests are failing. |
I know table name convention. But, how can I use unconventional pivot table name? I didn't test the whole case. My mistake. But why close so quickly? Always do you? |
Hey @Jehong-Ahn, Please don't take offense, it's not personal. Closing issues/pull requests early is normal practice that helps maintainers focus on bugs and features that require attention by only leaving those issues open. Plus we can always reopen any issue/pull request for further discussion. Now on to reviewing this patch. I've made the same mistake before, don't change existing test fixtures unless you are fixing a bug. Instead, create a new draft file in the fixtures directory. I don't think this pull request is necessary because, correct me if I'm wrong, Laravel automatically uses the value of
class Membership extends Pivot
{
protected $table = 'custom_table_name';
}
class User extends Model
{
public function teams(): BelongsToMany
{
return $this->belongsToMany(Team::class) // <-- no need to add Pivot table name
->using(Membership::class) // <- because table name is derived from `Membership::$table`
->as('membership')
->withPivot('id')
->withTimestamps();
}
}
class Team extends Model
{
public function users(): BelongsToMany
{
return $this->belongsToMany(User::class) // <-- no need to add Pivot table name
->using(Membership::class) // <- because table name is derived from `Membership::$table`
->as('membership')
->withPivot('id')
->withTimestamps();
}
} Thanks again for your contributions. |
Thanks, @ghostwriter Unfortunately, laravel doesn't use table name property for pivot relation. Maybe this issue is on the laravel, not blueprint. // If no table name was provided, we can guess it by concatenating the two
// models using underscores in alphabetical order. The two model names
// are transformed to snake case from their default CamelCase also.
if (is_null($table)) {
$table = $this->joiningTable($related, $instance);
} |
No, it's not on Laravel's side. You're right, I believe I understand what you're looking for. It's seems to me like we need to implement something that tells blueprint to use custom table name that's passed to Only If
Adding a test that explicitly tests these requirement should get the rest of the tests passing and achieve what you need. |
My previous answer is based on the recommendations laravel/framework#29571 (comment) It's definitely worth discussing with Laravel internals to see if they will accept a pull request to make sure Laravel starts using the |
PRs that don't have the correct change, tests, or a feature I wish to support within Blueprint get closed. That's how it works. If you want, you are welcome to fork this package and maintain your own version. 👍 If there is something you feel should still be added, please leave a clear comment demonstrating the problem. |
OK. Thanks for commenting. 👍 |
Hi if you use something like this it will take the table name in the pivot class
|
Thank you @dansleboby |
belongsToMany() needs a table name always.