-
-
Notifications
You must be signed in to change notification settings - Fork 35
Use pivot model table name if not defined in the relation #216
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
Conversation
| $relation = $this->belongsToMany( | ||
| $relatedClass, | ||
| $definition['table'] ?? null, | ||
| $definition['table'] ?? $definition['pivotModel'] ?? null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, a pivot model definition should be considered more authoritative than a table definition, on the off-chance that someone specifies both in the config.
| $definition['table'] ?? $definition['pivotModel'] ?? null, | |
| $definition['pivotModel'] ?? $definition['table'] ?? null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing that ensures at this point that the pivot model defines the table...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjauvin not sure what you mean? If you specify a pivot model, Laravel will determine the table of that model in the standard way (either the $table property of the model, or conventionally using the model's name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but there's no way to know at that point if the pivot actually defines a table name.
If it doesn't, laravel's way of guessing the table name is not compatible with ours
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's an example of this not working?
We already have a call to the using method underneath where you've made your edit, which should switch the relation to use the pivot's table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bennothommo I changed the order as you suggested, and added a [failing] test to demonstrate my point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjauvin sorry, but I'm still not understanding how this is an issue?
Your failing test is creating a BTM relation called pivot_without_table and using the CustomPivotWithoutTable class as a custom pivot without defining a table, and then expecting the table name to be custom_pivot_table, when by the normal automatic table name logic, it should be custom_pivot_without_table which is correctly being returned by the getTable() method.
It should be said that when defining a BTM relation in Laravel, you define either the table OR the pivot, you do not define both. If your pivot needs to use a table name that doesn't match the convention, then you specify the table name in the pivot.
EDIT: Never mind, it was buried deep in Laravel but evidently I'm wrong. The relation can still set the table name for a custom pivot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the test to show that it passes with the expected table name.
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
If using a custom pivot model, the table name may be omitted in the relation definition if it is set in the custom pivot model itself.
Laravel will resolve the table name to the Pivot model table.
(ref. https://github.com/laravel/framework/blob/12.x/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php#L184-L201)