laravel / pennant

A simple, lightweight library for managing feature flags.
https://laravel.com/docs/pennant
MIT License
482 stars 49 forks source link

[1.x] Allow retrieving all features for a scope when some features are defined for differing scopes #121

Closed cosmastech closed 3 weeks ago

cosmastech commented 3 months ago

This should resolve https://github.com/laravel/pennant/issues/112

We are building a feature flags endpoint for our mobile and web consumers. We need to be able to get all features for 3-4 scopes which are related to the user, but not the user object itself.

I ran into this exact issue earlier today. I saw that this issue had been open for a few weeks without a PR so I figured I'd take a stab at it.


Given we have features defined for different scopes

Feature::define('feature-for-user', fn(User $u) => true);
Feature::define('feature-for-team', fn(Team $t) => true);

When we attempt to load all features for a particular scope

$features = Feature::for($user)->all()

Then we expect to receive feature flag definitions ONLY for that apply to Users.

dd($features);
/*
array:1 [
  "feature-for-user" => true
]
*/

Currently, however, an exception is thrown: TypeError: Tests\Feature\DatabaseDriverTest::Tests\Feature{closure}(): Argument #1 ($t) must be of type Workbench\App\Models\Team, Workbench\App\Models\User given, called in /Users/luke/Projects/laravel-pennant/src/Drivers/Decorator.php on line 173

Non-goals of this PR

One outstanding question is: do we expect to allow for union or intersection types in the resolving function? Like Feature::define('team-or-user-feature', fn(Team|User $v) => true); Seems like this probably wouldn't work with how scopes are set up in general. I am sure that my change will not work in this case.

Also, you can still call Feature::for($user)->activate('team-scoped-feature'); I believe that ideally there would be a guard for this, but it seems like it's beyond the scope of this PR.

github-actions[bot] commented 3 months ago

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

taylorotwell commented 3 months ago

Drafting pending review from @timacdonald

timacdonald commented 3 weeks ago

@cosmastech, I have re-worked this one. There were a few problems with the original implementation. The main issue was when a value was resolved for a bad type, it was saved to the database.

The new implementation does the check before attempting to resolve. Supports union and intersection types.

What do you think?

Feature::for($scope)->all() and Feature::for($scope)->loadAll() will now only return or load the features for that scope type.

cosmastech commented 3 weeks ago

@cosmastech, I have re-worked this one. There were a few problems with the original implementation. The main issue was when a value was resolved for a bad type, it was saved to the database.

The new implementation does the check before attempting to resolve. Supports union and intersection types.

What do you think?

Feature::for($scope)->all() and Feature::for($scope)->loadAll() will now only return or load the features for that scope type.

Thanks for working on this @timacdonald. Sad you removed my given-when-then 😆 but otherwise, nice.

I didn't run any local testing on this, but it seems to solve the problem. Good catch on the DB queries, didn't consider that fact.