Open cosmastech opened 2 months ago
@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.
I want to be able to check if any of a user's related models have a particular feature enabled.
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:
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.