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

Adds setRoleClass method to PermissionRegistrar #1867

Merged

Conversation

timschwartz
Copy link
Contributor

Issue #1864

Adding this to allow configuration of Role model class from a package's service provider.

@erikn69
Copy link
Contributor

erikn69 commented Sep 19, 2021

You have to keep in mind that this package uses cache and preload models in memory, and if you are going to change the models in the runtime(after cache load), maybe you could have unexpected failures because models from cache will be default ones(is what I understood in #1864), maybe you could add some test to check that

@timschwartz
Copy link
Contributor Author

timschwartz commented Sep 19, 2021

Would duplicating the existing tests/RoleTest.php and modifying its setUp() to use a runtime configured model be the way to do that? I've never written a test before.

@PaolaRuby
Copy link
Contributor

PaolaRuby commented Sep 23, 2021

I think these tests are unnecessary, because they check the same as existing tests

Maybe you do not need a test because it is possible that only you need that method

@erikn69
Copy link
Contributor

erikn69 commented Sep 23, 2021

I don't mean duplicating the existing tests/RoleTest.php and modifying its setUp(), and those tests also have errors, did you run ./vendor/bin/phpunit?

ERRORS!
Tests: 377, Assertions: 794, Errors: 1, Failures: 1.

@erikn69
Copy link
Contributor

erikn69 commented Sep 23, 2021

@drbyte also maybe better

use Spatie\Permission\Contracts\Permission;
use Spatie\Permission\Contracts\Role;
    public function setPermissionClass($permissionClass)
    {
        $this->permissionClass = $permissionClass;
        config()->set('permission.models.role',  $permissionClass);
        app()->bind(Permission::class, $permissionClass); 
        return $this;
    }
    public function setRoleClass($roleClass)
    {
        $this->roleClass = $roleClass;
        config()->set('permission.models.role',  $roleClass);
        app()->bind(Role::class, $roleClass);  
        return $this;
    }

@timschwartz
Copy link
Contributor Author

I don't mean duplicating the existing tests/RoleTest.php and modifying its setUp(), and those tests also have errors, did you run ./vendor/bin/phpunit?

ERRORS!
Tests: 377, Assertions: 794, Errors: 1, Failures: 1.

Sorry, I pushed that to my branch just to save my work, I wasn't finished yet. I will read over the rest of the replies and fix it up.

@timschwartz timschwartz force-pushed the add_method_to_set_role_class branch from eb615cc to be6d9b2 Compare September 24, 2021 20:55
@timschwartz
Copy link
Contributor Author

@erikn69 Do I need to make anymore modifications to this?

@erikn69
Copy link
Contributor

erikn69 commented Oct 1, 2021

I'm just trying to help, you have to wait for a collaborator to review the pr, they haven't connected lately

@freekmurze freekmurze merged commit f1bdffd into spatie:main Oct 28, 2021
@freekmurze
Copy link
Member

Thanks!

@timschwartz timschwartz deleted the add_method_to_set_role_class branch October 28, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants