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

Temporary fix for incorrect role class name #313

Closed
wants to merge 1 commit into from
Closed

Temporary fix for incorrect role class name #313

wants to merge 1 commit into from

Conversation

pr4xx
Copy link

@pr4xx pr4xx commented Jun 22, 2018

Related to issue #306
After some intense digging, I might have found the problem. Whenever I run more than one phpunit test at once, Bouncer failes to fetch the list of abilities for current authenticated user. This only happens when using a custom role model with Bouncer::useRoleModel(Role::class);.
When using the custom role model, the getMorphClass() function of laravels HasRelationships.php returns the key roles instead of the actual class name (but only starting at the second test while running phpunit!).
Because of that, Bouncers getRoleConstraint() query failes to find the abilities since the tables entitiy_type column contains the custom role models class name instead of the returned roles string from the morph map.
I am not entirely sure why you used Models::role()->getMorphClass() since I did not fully understand the concept yet. However, my change to this line eliminates the error for now while passing your unit tests.

@JosephSilber
Copy link
Owner

This change is wrong. Simply hard-coding this here is going to break more things than it's going to fix.

I don't understand the original problem, and until I do, I can't fix it. As I've asked on the original issue, we need a minimal reproduction of the issue to get to the bottom of it.

I'm closing this for now. Let's continue the discussion on the original issue.

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.

2 participants