[Bug] Avoid checking permissions-via-roles on Role model (ref Model::preventAccessingMissingAttributes())

[juliomotol]

This PR aims to fix an issue when trying to check for Role's hasPermissionTo() when Model::preventAccessingMissingAttributes() is enabled.

The implemented fix overrides the HasPermission::getAllPermissions() to not check for $this->roles as the Role model will never have a relationship to itself.

[erikn69]

Why don't change getAllPermissions() on src/Traits/HasPermissions.php#L308-L318

Try this:

- if ($this->roles) {
// Try this way
+ if (method_exists($this, 'roles')) {
// Or maybe, i am not sure
+ if (isset($this->roles)) {

public function getAllPermissions(): Collection
{
    /** @var Collection $permissions */
    $permissions = $this->permissions;

    if (method_exists($this, 'roles')) {
        $permissions = $permissions->merge($this->getPermissionsViaRoles());
    }

    return $permissions->sort()->values();
}

laravel-permission/src/Traits/HasPermissions.php

Lines 308 to 318 in dc9c48c

	public function getAllPermissions(): Collection
	{
	/** @var Collection $permissions */
	$permissions = $this->permissions;
	if ($this->roles) {
	$permissions = $permissions->merge($this->getPermissionsViaRoles());
	}
	return $permissions->sort()->values();
	}

[juliomotol]

I'm down to do that. But what about this:

// HasPermissions.php
public function getAllPermissions(): Collection
{
    /** @var Collection $permissions */
    $permissions = $this->permissions;

    if (in_array(HasRoles::class, class_uses($this))) { // Check for the `HasRoles` trait
        $permissions = $permissions->merge($this->getPermissionsViaRoles());
    }

    return $permissions->sort()->values();
}

Let me know your thoughts.

[parallels999]

if (method_exists($this, 'roles')) {

Seems to be working

also maybe better

if (method_exists($this, 'getPermissionsViaRoles')) {
    $permissions = $permissions->merge($this->getPermissionsViaRoles());
}

[drbyte]

I'm inclined to think that if (method_exists($this, 'getPermissionsViaRoles')) { is the most semantically relevant. Certainly quite readable.
I imagine it's equally performant.

[erikn69]

I'm inclined to think that if (method_exists($this, 'getPermissionsViaRoles')) { is the most semantically relevant.

I think the same, it looks better, but do it works the same?

[juliomotol]

That would always result to true as getPermissionViaRoles() is also defined within HasPermissions

laravel-permission/src/Traits/HasPermissions.php

Lines 297 to 303 in dc9c48c

	public function getPermissionsViaRoles(): Collection
	{
	return $this->loadMissing('roles', 'roles.permissions')
	->roles->flatMap(function ($role) {
	return $role->permissions;
	})->sort()->values();
	}

[parallels999]

@juliomotol how about if (method_exists($this, 'roles')) {??

[juliomotol]

PR has been updated

[lloricode]

while not approved this, this is temporary fix

class Role extends \Spatie\Permission\Models\Role 
{

    protected bool $roles = false;

[juliomotol]

@drbyte Any plans on releasing this?

[drbyte]

Thanks!