pennant
pennant copied to clipboard
[1.x] Adds `Feature::activeForAny()`
I want to be able to check if any of a user's related models have a particular feature enabled.
$schools = $user->loadMissing('schools');
$featureIsActive = Feature::for($schools)->someAreActive('enrolled-in-beta');
But this actually is checking that all of the schools have at least one of the features (in this case it's just one feature, 'enrolled-in-beta', which is gets wrapped as an array) being passed in. So if all of the user's schools don't have the feature enabled, $featureIsActive is false.
With the change in this PR, we can accomplish that with:
$featureIsActive = Feature::for($schools)->activeForAny('enrolled-in-beta');
I would have just changed someAreActive() but it seems like that would be a breaking change (proven by the fact I would have to modify tests) 😢 hence the new method.
@cosmastech, thinking out loud here, I feel like the API we really want here is something like this:
Feature::forSome($schools)->active('enrolled-in-beta');
This better indicates that the "some" applies to the schools and not the features.
$school1 = School::find(1);
$school2 = School::find(2);
Feature::forSome([$school1, $school2])->active('enrolled-in-beta'); // false
Feature::for($school2)->activate('enrolled-in-beta');
Feature::forSome([$school1, $school2])->active('enrolled-in-beta'); // true
We then have better composition, for example we could mix forSome with someAreActive:
$model1 = Model::find(1);
$model2 = Model::find(2);
Feature::forSome([$model1, $model2])->someAreActive(['feature-1', 'feature-2']); // false
Feature::for($model1)->activate('feature-2');
Feature::forSome([$model1, $model2])->someAreActive(['feature-1', 'feature-2']); // true
Thoughts?
That's a great call @timacdonald. My first thought was "should we make scopes Enumerable?"
Feature::for($models)->some()->active('feature1');
// or even
Feature::for($models)->some->active('feature1');
But looking at Enumerable interface, I imagine most of those wouldn't apply. So maybe just methods for some() and every() would be sufficient.
edit: plus adding that Feature::forSome($models) helper method :+1:
I just realized that this doesn't make any sense:
Feature::forSome(['taylor', 'tim'])->activate('foo'); // or deactivate
What do we do in that case? Throw an exception? Just activate them for both scopes? Maybe throw each scope into a lottery?
My thinking is that the composition makes it more confusing here. @timacdonald I have started on an implementation that would allow for querying by some(), but I'm starting to feel like adding explicit methods will cause less headaches.
@cosmastech, I don't think all methods would be available. Only a subset of methods would be available after calling forSome.
I'm on something else at the moment, but will circle back to this. I don't want you to put in a heap of work on something we aren't sure on just yet.
@cosmastech, I'm going to close this one. This isn't a "no", more a "maybe later".
I don't think this is the API we want and we also want to see that more people want the feature before investing time in getting things right for it.
Thanks for the PR!