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

Add once() Helper Function To Role and Permission Checking. #672

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

onairmarc
Copy link

This PR adds the new once() helper function to Laratrust along with the option to enable/disable it via the configuration file.

This is useful as it removes the need to override the hasRole() and hasPermission() methods on the user model and instead moves this logic into Laratrust itself.

The default is that the use of this function is disabled due to the once() function not being added into Laravel Core until Laravel 11. The inline documentation comment with this configuration states that the application must be running Laravel 11 in order to use this function. The comment also states that if the application is running Laravel 10 or below, they need to run composer require spatie/once

@santigarcor
Copy link
Owner

I like the idea, could you please add the tests for it?

Also, I have a question, does the once method takes into consideration if the callback is using different params?

@onairmarc
Copy link
Author

@santigarcor I can try to add these tonight or tomorrow. My understanding is that if different values are passed to once() then the result will be different from the last time once() was called, But if you pass call once(1, 2, 3) multiple times, the DB lookup will only occur once.

Please let me know if that answers your question or if I can explain it better.

@onairmarc
Copy link
Author

@santigarcor I just added the configs into the already existing UserTest.php. From what I can see, this should test the expected behavior. Please let me know if I'm misunderstanding anything.

@santigarcor
Copy link
Owner

No, those lines you added are not testing anything there. You should place them here. that's the base class for all checks.

@onairmarc
Copy link
Author

@santigarcor I believe I added the tests to the proper file. Can you please verify that this is what you wanted me to do? If there's anything else, please let me know.

@onairmarc
Copy link
Author

@santigarcor looking at the tests, I see that only the Laravel 10 tests are failing, which I would expect since once is not included in Laravel 10 and this package doesn't require spatie/once.

I'm not sure what the best method would be for getting the Laravel 10 tests to pass. Is there a way to detect the version of Laravel being run in the tests? The only thing that comes to mind is the Composer Runtime API.

Thoughts?

@santigarcor
Copy link
Owner

I think we need to find another solution for this, because if we release this for version 8.x it has support for 10.x and 11.x. I won't do it for version 9.x because i'm thinking on a rewrite.

@onairmarc
Copy link
Author

onairmarc commented May 2, 2024

@santigarcor So my brain completely forgot about the function_exists() PHP function. I added this to the Config::get('laratrust.cache.once') check. So now the developer will need to enable laratrust.cache.once and the once() function will need to be registered.

once() registration will occur by installing the spatie/once package in L10 and automatically in L11. As far as the tests are concerned, once() does not exist in the L10 tests, so the current behavior will not be altered to use once() even if laratrust.cache.once is set to true

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.

None yet

2 participants