Skip to content

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.

Sign up for GitHub

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

Closed
mjauvin wants to merge 8 commits into from
+56 −2
Closed

Use pivot model table name if not defined in the relation #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
efe85fc
Use pivot model table name if not defined in the relation
mjauvin May 17, 2025
84ae111
add unit test for table defaulting to custom pivot model table
mjauvin May 17, 2025
ef09bca
add missing public keyword
mjauvin May 17, 2025
e4cbd68
pivotmodel first
mjauvin May 21, 2025
99d72df
Demonstrate correct able name
bennothommo May 22, 2025
1a35249
Marc was right, Ben was wrong
bennothommo May 22, 2025
68f8f3d
also restore BelongsToMany relation table assignment order
mjauvin May 22, 2025
3d5e806
add an extra test to make sure relation table has precedence
mjauvin May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Database/Concerns/HasRelationships.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ protected function handleRelation(string $relationName, bool $addConstraints = t
case 'belongsToMany':
$relation = $this->belongsToMany(
$relatedClass,
$definition['table'] ?? null,
$definition['table'] ?? $definition['pivotModel'] ?? null,

@bennothommo bennothommo May 19, 2025

Copy link
Copy Markdown
Member

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.

Suggested change
$definition['table'] ?? $definition['pivotModel'] ?? null,
$definition['pivotModel'] ?? $definition['table'] ?? null,

@mjauvin mjauvin May 19, 2025

Copy link
Copy Markdown
Member Author

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...

@bennothommo bennothommo May 21, 2025

Copy link
Copy Markdown
Member

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).

@mjauvin mjauvin May 21, 2025

Copy link
Copy Markdown
Member Author

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

@bennothommo bennothommo May 21, 2025

Copy link
Copy Markdown
Member

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.

@mjauvin mjauvin May 21, 2025

Copy link
Copy Markdown
Member Author

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.

@bennothommo bennothommo May 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@bennothommo bennothommo May 22, 2025

Copy link
Copy Markdown
Member

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.

$definition['key'] ?? null,
$definition['otherKey'] ?? null,
$definition['parentKey'] ?? null,
Expand Down Expand Up @@ -481,7 +481,7 @@ protected function handleRelation(string $relationName, bool $addConstraints = t
$relation = $this->morphToMany(
$relatedClass,
$definition['name'] ?? $relationName,
$definition['table'] ?? null,
$definition['table'] ?? $definition['pivotModel'] ?? null,
$definition['key'] ?? null,
$definition['otherKey'] ?? null,
$definition['parentKey'] ?? null,
Expand Down
54 changes: 54 additions & 0 deletions tests/Database/Relations/BelongsToManyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Winter\Storm\Tests\Database\Relations;

use Winter\Storm\Database\Model;
use Winter\Storm\Database\Pivot;
use Winter\Storm\Support\Facades\DB;
use Winter\Storm\Tests\Database\Fixtures\Category;
use Winter\Storm\Tests\Database\Fixtures\Post;
Expand Down Expand Up @@ -369,4 +370,57 @@ public function testConditionsWithPivotAttributes()
$this->assertEquals([1, 2], $author->executiveAuthors()->lists('id'));
$this->assertEquals([1, 2], $author->executiveAuthors()->get()->lists('id'));
}

public function testTableDefaultsToCustomPivotTable()
{
$model = new TestModel();
$model->addBelongsToManyRelation('pivot_with_table', [
Model::class,
'pivotModel' => CustomPivotWithTable::class,
]);

$relation = $model->pivot_with_table();

$this->assertEquals('custom_pivot_table', $relation->getTable());
}

public function testTableDefaultsToCustomPivotTableWithoutTable()
{
$model = new TestModel();
$model->addBelongsToManyRelation('pivot_without_table', [
Model::class,
'pivotModel' => CustomPivotWithoutTable::class,
]);

$relation = $model->pivot_without_table();

$this->assertEquals('custom_pivot_without_table', $relation->getTable());
}

public function testTableDefaultsToRelationTable()
{
$model = new TestModel();
$model->addBelongsToManyRelation('pivot_without_table', [
Model::class,
'table' => 'custom_pivot_table',
'pivotModel' => CustomPivotWithoutTable::class,
]);

$relation = $model->pivot_without_table();

$this->assertEquals('custom_pivot_table', $relation->getTable());
}
}

class TestModel extends Model
{
}

class CustomPivotWithTable extends Pivot
{
public $table = 'custom_pivot_table';
}

class CustomPivotWithoutTable extends Pivot
{
}