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

Avoid calling the config helper in the role/perm model constructor #2098

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

adiafora
Copy link
Contributor

@adiafora adiafora commented May 17, 2022

I described the problem in #2097 with initializing the config.
I propose a solution.

Fixes #2097

@erikn69
Copy link
Contributor

erikn69 commented May 17, 2022

Closes #2097
What about Permission model ?

$attributes['guard_name'] = $attributes['guard_name'] ?? config('auth.defaults.guard');

Could you test with facade \Config::get('auth.defaults.guard'), (Config.php)

@adiafora
Copy link
Contributor Author

@erikn69 I think it needs fixing too. I'll do it

@adiafora
Copy link
Contributor Author

I tried. The facade doesn't work either

@erikn69
Copy link
Contributor

erikn69 commented May 18, 2022

#2097 (comment)
Ok , i did test your solution and it don't do anything, look at this example

class Kernel extends ConsoleKernel
{
    public function __construct(
        Application $app,
        Dispatcher $events,
        private \Spatie\Permission\Models\Role $role
    ){
        parent::__construct($app,$events);
        var_dump($role->toArray(), (new \Illuminate\Config\Repository())->get('auth'));
    }
}

From that i am getting

php artisan cache:clear
array(1) {   
  ["guard_name"]=> NULL          # this is $role class
}
NULL                             # this is Config\Repository()
Application cache cleared!

It seems that accessing Config\Repository before container creation does not get any config values


I think that we can delete that line without problems, it seems to me that those lines are forgotten code from previous versions, it was replaced by Guard::getDefaultName on other methods

$attributes['guard_name'] = $attributes['guard_name'] ?? Guard::getDefaultName(static::class);

I did run test without those lines and everything works

PHPUnit 9.5.20

Runtime:       PHP 8.1.2
Configuration: /laravel-permission/phpunit.xml.dist

...............................................................  63 / 414 ( 15%)
............................................................... 126 / 414 ( 30%)
............................................................... 189 / 414 ( 45%)
............................................................... 252 / 414 ( 60%)
............................................................... 315 / 414 ( 76%)
............................................................... 378 / 414 ( 91%)
....................................                            414 / 414 (100%)

Time: 00:07.732, Memory: 48.00 MB

OK ( 414 tests, 928 assertions)

@adiafora
Copy link
Contributor Author

adiafora commented May 19, 2022

@erikn69 I did. Could you take a look?

@erikn69
Copy link
Contributor

erikn69 commented May 19, 2022

I think that we can delete that line without problems,
I did run test without those lines and everything works

Read again

@adiafora
Copy link
Contributor Author

I'm sorry. I misunderstood.
I did.

@adiafora
Copy link
Contributor Author

ok, I did

@adiafora
Copy link
Contributor Author

Did

@angeljqv
Copy link
Contributor

@freekmurze ping

@drbyte drbyte changed the title In the model, access the config through a class, not through a helper Avoid calling the config helper in the role/perm model constructor Oct 19, 2022
@drbyte drbyte merged commit 5e96846 into spatie:main Oct 19, 2022
@JonPurvis
Copy link

JonPurvis commented Oct 19, 2022

Is it possible that this change is the cause of #2218?

I've switched my app from using v5.5.9 to v5.5.10 and the latter now has the above issue, switching back to v5.5.9 resolves it.

@erikn69
Copy link
Contributor

erikn69 commented Oct 19, 2022

@JonPurvis yes, it was

Try mi last PR

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.

Fatal error: Uncaught ReflectionException: Class "config" does not exist
5 participants