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

Pivot models $table variable not respected / overridden #51410

Closed
shortontech opened this issue May 14, 2024 · 8 comments
Closed

Pivot models $table variable not respected / overridden #51410

shortontech opened this issue May 14, 2024 · 8 comments

Comments

@shortontech
Copy link

shortontech commented May 14, 2024

Laravel Version

11.7.0

PHP Version

8.3.6

Database Driver & Version

mysql-community-server 8.0.35-1ubuntu22.04

Description

I created a pivot model to connect two tables, and set the protected $table attribute to a different table name that exists in my database. When attaching records, the $table property was not respected, and the getTable function must be overridden.

If no table is passed to Illuminate\Database\Eloquent\Concerns\HasRelationships::belongsToMany it generates a new table name and (eventually) sets the table name of the model to that value once that value is passed to `Illuminate\Database\Eloquent\Relations\Concerns\AsPivot::fromAttributes.

There is no check on the model to ensure that the table name is set, and while I might imagine there could be concerns that the one writing the relationship would like to override the table, resolving the table name should be done later so that there's the possibility that it may be left to the Pivot once instantiated, given that the same table name resolution behavior is present in Illuminate\Database\Eloquent\Relations\BelongsToMany::resolveTableName and Illuminate\Database\Eloquent\Relations\Concerns\AsPivot::getTable

That is to say, they both transform the model name to snake case in singular form.

There is an undocumented behavior, in that you can pass a Pivot instance to the $table parameter, which does not run into this issue, but the name of the variable gives no hint at this behavior (and is undocumented).

Steps To Reproduce

  • Create two models
  • Use a belongsToMany relationship with a Pivot class $this->belongsToMany($relation)->using(PivotClass::class)
  • Override private $table on the Pivot class
  • Attempt to attach the models together using the Pivot.

I created the two regular models and one pivot model to connect the two, and when attaching one to the other I get the error:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'my_proj.card_category' doesn't exist (Connection: mysql, SQL: insert into `card_category` (`category_id`, `card_id`) values (3, ?))

When I use tinker to check the table name, everything appears normal, which means that the Pivot model's $table attribute is being modified by setTable()

Psy Shell v0.12.3 (PHP 8.3.6 — cli) by Justin Hileman
> app(CardCategory::class)->getTable()
[!] Aliasing 'CardCategory' to 'App\Models\CardCategory' for this Tinker session.
= "card_categories"

App\Models\CardCategory:

<?php
namespace App\Models;
use Illuminate\Database\Eloquent\Relations\Pivot;
class CardCategory extends \Illuminate\Database\Eloquent\Relations\Pivot {
    protected $table = "card_categories";
}

App\Models\Card:

<?php
namespace App\Models;
class Card extends \Illuminate\Database\Eloquent\Model
{
    public function categories()
    {
        return $this->belongsToMany(Category::class)->using(CardCategory::class);
    }
}
Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@driesvints
Copy link
Member

Could you maybe try the solution from this issue? #48277

@edwwaarrdd
Copy link

edwwaarrdd commented May 17, 2024

Hey @driesvints I started work on a possible fix but unsure if it's the right way to go forward.
I left it as a draft for now as the ->attach() works as expected but the get functionality is still broken. :(

@driesvints
Copy link
Member

Thanks @edwwaarrdd. To be very honest, often when Pivot models are stretched like this we recommend to just use a dedicated eloquent model. I'm not sure if we should do any major changes.

@staudenmeir
Copy link
Contributor

@shortontech @edwwaarrdd This is a very fundamental issue that can't be solved (without dirty hacks):
Calling $this->belongsToMany(Category::class) already creates the relationship's base query and so applying ->using() afterwards doesn't change the pivot table anymore.

There is an undocumented behavior, in that you can pass a Pivot instance to the $table parameter, which does not run into this issue, but the name of the variable gives no hint at this behavior (and is undocumented).

I agree that the solution should be documented, as this topic comes up regularly.

@edwwaarrdd
Copy link

@staudenmeir Yes I agree with this. Started tinkering a little bit but it became clear pretty fast that it's not something easy to fix.

So @shortontech if you want to fix your issue instead of declaring your belongs to many like this:

public function categories()
    {
        return $this->belongsToMany(Category::class)->using(CardCategory::class);
    }

You can write it this way:

public function categories()
    {
        return $this->belongsToMany(Category::class, CardCategory::class);
    }

And it will work the same as ->using() but the $table variable will be respected as it calls the using() function in the construct of the belongsToMany class.

I am going to close my PR

Thanks for the quick responses @driesvints & @staudenmeir

@driesvints
Copy link
Member

Yeah I think we should as well. Thanks @staudenmeir. Would appreciate a PR to the docs if possible!

@edwwaarrdd
Copy link

Added a small extra section for it in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants