laravel / pennant

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

resolve() doesn't receive instances of FeatureScopeable as the scope argument. #88

Closed SamMoffat closed 7 months ago

SamMoffat commented 8 months ago

Hi!

I ran into an issue when implementing FeatureScopeable on an object and then resolving a non-cached feature flag.

When I defined a feature, if I then had Feature::for($scopeable)->active('feature-I-defined'), the scope would be of the model. When I defined an object with FeatureScopeable, the scope was the serialised scope string, and not the object itself.

The issue seemed to stem from the Decorator's resolveScope() method. As this was returning the feature identifier instead of the scope if $scope implemented FeatureScopeable. This works for when you want to read from/write to the driver layer, but it means that the object can't be used for any feature resolving if the object isn't a model.

In this branch, I have made the following changes:

I'm a little concerned that FeatureScopeable objects' behaviour has changed for the drivers, but I think this is a move towards more expected behaviour. In the provided drivers and redis driver I found, the drivers already handle serialisation.

Let me know your thoughts and feedback.

Kind regards, Sam Moffat

timacdonald commented 7 months ago

@SamMoffat, nice to see you building a Redis driver!

The behaviour you are having troubles with is the intended behaviour of the package.

Drivers are not supposed to receive the original object. If a user-land object implements FeatureScopeable, they are saying "please use the return type I provide as my scope for any feature related to me". So the decorator will resolve the "scope" from the object and use that to respect the instruction.

You could imagine a library of feature flags that all rely on an email address.

Feature::define('new-login', fn (string $email) => Str::endsWith($email, '@laravel.com'));

$user = User::first();

Feature::for($user->email)->active();

When using the feature scopeable interface, it allows the codebase to pass the user in place of the email address and have the email resolved under the hood.

class User extends Model implements FeatureScopeable 
{
    public function toFeatureIdentifier(string $driver): mixed
    {
        return $this->email;
    }
}

These two calls are now exactly the same.

Feature::for($user->email)->active();
Feature::for($user)->active();

If your driver needs specific handling, you can document the requirement by having the user check the $driver value in the toFeatureIdentifier method.

class User extends Model implements FeatureScopeable 
{
    public function toFeatureIdentifier(string $driver): mixed
    {
        if ($driver === 'redis') {
            return // ...
        }

        return $this->email;
    }
}

If you feel I'm missing something, please let me know. Alternatively, if you could provide a failing test so I can better understand the problem, that would be fantastic.