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

Role::getTable() method brokes withCount method #2277

Closed
xenaio-daniil opened this issue Dec 18, 2022 · 1 comment · Fixed by #2280
Closed

Role::getTable() method brokes withCount method #2277

xenaio-daniil opened this issue Dec 18, 2022 · 1 comment · Fixed by #2280

Comments

@xenaio-daniil
Copy link
Contributor

xenaio-daniil commented Dec 18, 2022

I needed to get ManyToMany relation of roles (structure of departments). To do this, I extended the \Spatie\Permission\Models\Role class with methods:

class Department extends \Spatie\Permission\Models\Role
{
    ....
    public function parents(): BelongsToMany
    {
        return $this->belongsToMany(
            Department::class,
            "department_hierarchy,
            'child_id',
            'parent_id'
        );
    }

    public function children(): BelongsToMany
    {
        return $this->belongsToMany(
            Department::class,
            'department_hierarchy',
            'parent_id',
            'child_id'
        );
    }
    ....
}

and created migration:

    Schema::create('department_hierarchy', function (Blueprint $table) {
        $table->string("rel_id", 25)->primary();
        $table->bigInteger("parent_id", false, true);
        $table->bigInteger("child_id", false, true);
        $table->boolean("is_direct")->default(false);
        $table->foreign("parent_id")->references("id")->on("roles");
        $table->foreign("child_id")->references("id")->on("roles");
    });

Everything works well except methods like

\App\Models\Department::withCount('children')

and so on: it always returns parent_count = 0.

The reason of this behavior is in overriding of method getTable in Spatie\Permission\Models\Role.

When extending Illuminate\Database\Eloquent\Model:

echo \App\Models\Department::withCount('children')->toSql();
/**
SELECT `roles`.*, (
SELECT COUNT(*)
FROM `roles` AS `laravel_reserved_0`
INNER JOIN `department_hierarchy` 
    ON `laravel_reserved_0`.`id` = `department_hierarchy`.`child_id`    <-- look at laravel_reserved_0
WHERE `roles`.`id` = `department_hierarchy`.`parent_id`) AS `children_count`
FROM `roles`

When extending \Spatie\Permission\Models\Role:

echo \App\Models\Department::withCount('children')->toSql();
/**
SELECT `roles`.*, (
SELECT COUNT(*)
FROM `roles` AS `laravel_reserved_0`
INNER JOIN `department_hierarchy` 
    ON `roles`.`id` = `department_hierarchy`.`child_id`    <------ laravel_reserved_0 != roles
WHERE `roles`.`id` = `department_hierarchy`.`parent_id`) AS `children_count`
FROM `roles`

withCount() uses Model::table field to temporary store alias name (laravel_reserved_0).

And when (in Illuminate\Database\Eloquent\Model) method Model::getTable() returns $this->table or magic name from class basename, your version returns only pre-configurated table name (config('permission.table_names.roles')) or, if configuration not found, Model::getTable(). So if permission.table_names.roles is configured, it has no change to return $this->table.

I would recommend to rewrite this method like this:

    public function getTable()
    {
        return $this->table ?? config('permission.table_names.roles', parent::getTable());
    }

or for best practice remove method Role::getTable() and replace it with

    public function __construct(array $attributes = [])
    {
        $attributes['guard_name'] = $attributes['guard_name'] ?? config('auth.defaults.guard');

        parent::__construct($attributes);

        $this->guarded[] = $this->primaryKey;

        $this->table = config('permission.table_names.roles', parent::getTable()); // <---------------
    }
@parallels999
Copy link
Contributor

Feel free to open a PR

xenaio-daniil added a commit to xenaio-daniil/laravel-permission that referenced this issue Dec 20, 2022
drbyte pushed a commit that referenced this issue Feb 8, 2023
* Fix Role::withCount if belongsToMany declared

fixes #2277
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants