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

Remove null checks and allow it to be queried #608

Merged
merged 7 commits into from
Feb 10, 2023

Conversation

grantholle
Copy link
Contributor

Fix #607. This has the expected behavior and not so eager to grant permissions when you may not want them to be granted.

@grantholle
Copy link
Contributor Author

Also, if you don't mind adding the hacktoberfest-accepted tag to this, it would be really helpful in my goal of contributing during Hacktoberfest. Thanks!

@grantholle
Copy link
Contributor Author

I've ran into the same situation when checking roles assignment. I have a scoped assigned role, but when I check $user->isA($role) when there isn't a scope set, it still passes.

I will try to include an update for that.

@grantholle
Copy link
Contributor Author

Hi @JosephSilber, any plans to get this merged? Thanks for taking the time!

src/Database/Scope/Scope.php Outdated Show resolved Hide resolved
src/Database/Scope/Scope.php Outdated Show resolved Hide resolved
@JosephSilber
Copy link
Owner

Whoops. I should've gotten to this way sooner, especially since you put in all that work creating the issue, the repro repo and a PR with tests! Sorry.

I left you a small review. Check it out, and let's get this merged.

@grantholle
Copy link
Contributor Author

Thanks man, I've made those suggested edits. No worries on the delay!

@grantholle
Copy link
Contributor Author

Never mind, my local repo was out of date with this. Anyway, you were right on all of the suggestions.

@JosephSilber JosephSilber merged commit 502221b into JosephSilber:master Feb 10, 2023
@JosephSilber
Copy link
Owner

Merged. Thanks!

@mikerockett
Copy link

Well, this broke my app, but in a good way because it turns out we had scopes set when they weren't even being used (best guess is experimentation 😅).

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

Successfully merging this pull request may close these issues.

Unexpected scoping behavior
3 participants