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

Using multiple scopes across features disables use of all() and values() #112

Open
joshrainwater opened this issue Jul 17, 2024 · 3 comments · May be fixed by #121
Open

Using multiple scopes across features disables use of all() and values() #112

joshrainwater opened this issue Jul 17, 2024 · 3 comments · May be fixed by #121
Assignees

Comments

@joshrainwater
Copy link

My project requires scoping features across multiple different models. Quick example:

Feature::define('beta-design', function(Team $team)...)
Feature::define('password', function(User $user)...)

When I configure this a Service provider, all calls to all() or values() functions throw 500 errors with incorrect parameter types. A call to Feature::for($user)->all() also throws errors.

I think my ideal in this situation would be instead to return a list of all features under the current model type scope, rather than throwing an error and checking them. The other alternative would be to return false for all features that are not under the scope of the model being tested. So either Feature::all() would not return any value for beta-design in this case, or it would return false.

I am happy to code this up and add a Reflection into PendingScopedFeatureInteraction (or drivers, if that makes more sense) but wanted to confirm which expected behavior seems better. Either way, I don't like working with a system where both of these functions are completely disabled.

@timacdonald timacdonald self-assigned this Jul 17, 2024
@timacdonald
Copy link
Member

Thanks, @joshrainwater.

I wrote up a big reply with my thinking on this behaviour and where would could and shouldn't change things...and in the process I think I convinced myself we may want ease restrictions across the board on Pennant ha!

Think I just need to sit on this one for a day or two to process my thoughts on the potential changes.

@timacdonald
Copy link
Member

I think I've come around to the idea that we should not throw exceptions when you try to pass a scope value to a feature definition that does not support it. Instead, we should just return false and trigger an event.

Feature::defined('user-feature', fn (User $user) => true);

//

Feature::for(User::first()->active('user-feature'); // true
Feature::for(Team::first()->active('user-feature'); // false

That solves the values issue, as they would just show up as false.

However, it would be best if the all function only returned features that are supported by the type.

Feature::defined('user-feature', fn (User $user) => true);
Feature::defined('team-feature', fn (Team $team) => true);

Feature::for(Team::first())->values([
    'user-feature',
    'team-feature',
]);

// [
//     'user-feature' => false,
//     'team-feature' => true,
// ]

Feature::for(Team::first())->all();

// [
//     'team-feature' => true,
// ]

What do you think, @joshrainwater?

@joshrainwater
Copy link
Author

Yeah I like this; let's me still use some of the shortcut functions in cases where I just want to grab a list of all Team-level features in the few cases that it's the best thing to do.

Also adding an event trigger for trying to get a feature for the wrong scope eliminates some of the hesitation around ignoring a mistake being made.

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