laravel / pennant

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

Cannot use non-eloquent objects as scope #97

Closed jkatruska closed 7 months ago

jkatruska commented 7 months ago

Pennant Version

1.7.0

Laravel Version

10.15.0

PHP Version

8.1.23

Database Driver & Version

No response

Description

Problem

I'm currently using laravel without eloquent, and I'm trying to use simple user object as scope. Data to this object are loaded from auth API as I'm using service oriented architecture . When I try to use that object as scope I'm getting an exception Unable to serialize the feature scope to a string. You should implement the FeatureScopeable contract., so I've implemented this interface to my object but basically return type of that method must be null|int|string|Model or I'll still get said exception from \Laravel\Pennant\FeatureManager::serializeScope. So I had to change my implementation to return just userId . But now my feature resolvers are getting just that userId as scope which means I have to again get user from my auth service based on its id.

Expected behaviour

I want it to work like it works with eloquent models. So you can pass object as scope and it will get serialized to string for purpose of serialization scope for storage.

Proposed solution

At first I thought that we can add new arm to match in serializeScope() something like this $scope instanceof \Laravel\Pennant\Contracts\FeatureScopeable => $scope->toFeatureIdentifier() but then I've realised that this method may not return string and looking further into the code it could break some things. So as an alternative to this we could create new interface to serve this purpose to create identifier for serialization. Then you can implement it at any object and you can keep your object as scope and serialization would also work.

Steps To Reproduce

Create any POPO object and try to use it as a scope, at first you will get exception to use FeatureScopeable and when you will implement it you will still be getting exceptions if your return value won't be null|int|string|Model

driesvints commented 7 months ago

Right now this requires Eloquent, sorry. We don't have plans to change this right now. You can always attempt a PR to change this and depending on the amount of code involved and complexity we can see how it goes.

timacdonald commented 7 months ago

@jkatruska, you may use anything as scope, eloquent or not.

The value returned from the toFeatureIdentifier method should uniquely identify the scope in some serialized way.

For example, lets say I have the following user object...

final readonly class User
{
    public function __construct(
        public int $id,
        public string $name,
        public string $email,
    ) {
        //
    }
}

Notice that the class does not extend eloquent. I can use this object with Pennant manually...

$user = new User(
    id: 432,
    name: 'Tim',
    email: 'tim@laravel.com'
);

/* ... */

// use the integer based ID...

Feature::for($user->id)->active('new-api');

Notice I use the ID here and not the email, as the email address may change and would break the connection between the stored value and the user.

If I want to integrate with Pennant directly, as you are trying to do, I would implement the FeatureScopeable interface...

final readonly class User implements FeatureScopeable
{
    public function __construct(
        public int $id,
        public string $name,
        public string $email,
    ) {
        //
    }

    public function toFeatureIdentifier(string $driver): int
    {
        return $this->id;
    }
}

Then the object may be used with Pennant like so:

- Feature::for($user->id)->active('new-api');
+ Feature::for($user)->active('new-api');

If you want to make it easier to identify scopes in the database or make sure it does not conflict with other integer based scopes, you can prepend a value to the id, which is probably what I would do:

final readonly class User implements FeatureScopeable
{
    public function __construct(
        public int $id,
        public string $name,
        public string $email,
    ) {
        //
    }

    public function toFeatureIdentifier(string $driver): string
    {
        return "user:{$this->id}";
    }
}

Hope that helps.

jkatruska commented 7 months ago

Hi @timacdonald,

as I mentioned previously I'm aware of this, but this would result in that my feature resolver will get int or string as input parameter and I would have to fetch user again so I can call user methods e.g. $user->isInBeta() to determine if user can access some feature.

On the other hand if you pass Eloquent object as a scope you will get that same object in your resolver.