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

HasRolesAndAbilities trait conflict in Laravel 5.4.31 #187

Closed
jevets opened this issue Aug 2, 2017 · 3 comments
Closed

HasRolesAndAbilities trait conflict in Laravel 5.4.31 #187

jevets opened this issue Aug 2, 2017 · 3 comments
Labels

Comments

@jevets
Copy link

jevets commented Aug 2, 2017

  • Vanilla laravel install 5.4.31 (I simply used composer's global "laravel/installer") laravel new my-app
  • Default user tables/migrations
  • Default auth scaffold
  • Default App\User model
  • composer require silber/bouncer v1.0.0-beta.2 and install per instructions

Adding the HasRolesAndAbilities trait to default App\User model results in an error

// app/User.php

class User extends Authenticatable
{
    use HasRolesAndAbilities;
}
php artisan -V

[ErrorException]
Declaration of Silber\Bouncer\Database\HasRolesAndAbilities::isNot($role) should be compatible with Illuminate\Database\Eloquent\Model::isNot(Illuminate\Database\Eloquent\Mo
  del $model)

Digging a bit, it looks like Illuminate\Database\Eloquent\Model now defines its own isNot() method:

// Illuminate\Database\Eloquent\Model
// around line 1004

/**
 * Determine if two models are not the same.
 *
 * @param  \Illuminate\Database\Eloquent\Model  $model
 * @return bool
 */
public function isNot(Model $model)
{
    return ! $this->is($model);
}

HasRoles trait is responsible

This doesn't complain:

// app/User.php

use Silber\Bouncer\Database\Concerns\HasAbilities;

class User extends Authenticatable
{
    use HasAbilities;
}
php artisan -V
Laravel Framework 5.4.31

But use HasRoles does squak:

// app/User.php

use Silber\Bouncer\Database\Concerns\HasAbilities;
use Silber\Bouncer\Database\Concerns\HasRoles;

class User extends Authenticatable
{
    use HasAbilities;
    use HasRoles;
}
php artisan -V

 [ErrorException]
  Declaration of Silber\Bouncer\Database\Concerns\HasRoles::isNot($role) should be compatible with Illuminate\Database\Eloquent\Model::isNot(Illuminate\Database\Eloquent\Model
   $model)

I tried to override isNot() on my App\User model, but Bouncer's isNot() takes a (string) $role argument, while Model's isNot() expects a Model instance (isNot(Model $model) {}).

Not sure if there's an easy fix for this. I know it's possible to substitute trait-provided functions, but I don't know if/how much Bouncer uses isNot($role) internally.

In my case, I can probably just stick w/ Laravel 5.4.30.

@jevets
Copy link
Author

jevets commented Aug 2, 2017

I encountered a similar/related issue, though there's an easy enough workaround (just use the Bouncer facade). Thought it was worth mentioning here.

It affects Laravel 5.4.30. I assume it would affect 5.4.31 as well, though I haven't checked.

  • Laravel 5.4.30
  • Bouncer v1.0.0-beta.2

Laravel 5.4.30's \Illuminate\Database\Eloquent\Model class also has an is(Model $model) method, which conflicts with Bouncer's is($role)method.

$user = App\User::first();
// { id: 1, name: "Steve", ... }

$role = \Silber\Bouncer\Database\Role::first();
// { id: 1, name: "admin", ... }

// both result in error
$user->is('admin');
$user->is($role->name);

TypeError: Argument 1 passed to Illuminate\Database\Eloquent\Model::is()
must be an instance of Illuminate\Database\Eloquent\Model, string given on line 1

Eloquent\Model's is() method:

// Illuminate\Database\Eloquent\Model
// around line 990

/**
 * Determine if two models have the same ID and belong to the same table.
 *
 * @param  \Illuminate\Database\Eloquent\Model  $model
 * @return bool
 */
public function is(Model $model)
{
    // ...
}

Workarounds

1. Just use the Facade

It's easy enough to work around the problem by just using the facade instead of directly asking the $user instance. I'm sure some may dislike the Facade solution, but it works.

e.g.

$user = App\User::first();
// { id: 1, name: "Steve", ... }

$role = \Silber\Bouncer\Database\Role::first();
// { id: 1, name: "admin", ... }

Bouncer::is($user)->a($role->name);
// true
Bouncer::is($user)->an('admin');
// true

2. Just make your own method(s)

You could also simply define your own method(s) directly on your App\User model. I haven't done this or tested this, but you'll get the idea.

// app\User.php

class User
{
    public function hasRole($role)
    {
        if ($role instanceof \Silber\Bouncer\Database\Role) {
            $role = $role->name;
        }

        return \Bouncer::is($this)->a($role);
    }

    // public function hasAllRoles()
    // public function hasAnyRoles()
    // and so on...
}

I'm sure there are other approaches, but these two will work for you if you really need to check a user's role directly from an App\User instance.


I don't mind if these direct user role checks are deprecated or removed entirely; I agree that we should check for abilities instead of roles. I'm ok with Facades.

But the isNot() conflict on Laravel 5.4.31 prevents Bouncer from being fully installed.

I did spend some more time attempting to override the traits, substituting other traits/methods using insteadof and a, but I couldn't get it working. I'm not really sure what Bouncer's Clipboard does or even is.

@JosephSilber
Copy link
Owner

I'm afraid we'll have to do the same thing we did to is — we'll have to switch to using isNotA 😢

@jesephan
Copy link

jesephan commented Aug 3, 2017

@JosephSilber We are also having the same issue. For now we'll just downgrade to Laravel 5.4.0 anyway we are still on early part of the development. Hope to receive the bug fix soon when you have time. Thank you.

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

No branches or pull requests

3 participants