Deactivating Rich Feature results in data loss

[inmanturbo]

Pennant Version

1.15.0

Laravel Version

11.37.0

PHP Version

8.3.11

Database Driver & Version

mariadb Ver 15.1 Distrib 10.11.9-MariaDB, for debian-linux-gnu (x86_64)

Description

I wonder if it might be considered with a major release coming up, to break out a seperate boolen column for determining whether a feature is active in the database driver?

Currently we have a value column for storing a boolean OR rich schemaless json value.

The issue we are facing with this right now for a particular feature set is that we need the app to remember somehow what a user's value was set to for a feature before it was disabled, so that we can restore thier feature to the correct value if it gets reactivated in the future. This means we need some external system or store for tracking the feature values anyway, so that the rich feature values don't do us much good in this case.

Obviously purging features will result in all values becoming lost and that's a given, but we only purge during testing and development, then run scripts and seeders to simulate a realistic feature set.

I would be suprised at this point given the level of adoption and stability in the project if this would be considered. What I am wondering then is if the maintainers and @timacdonald et al might have any advice for implementing this in a custom driver, or perhap a simple extension?

Or maybe the maintainers would consider a PR for serializing the boolean into the value column alongside the optional rich value, seperated by a pipe character? 😉

name	scope	value
blog-theme	App\Models\User|1	true|"{\"font_color\":\"gray-900\", \"background_color\":\"gray-100\"}"
custom-font	App\Models\User|2	false|"Agu Display"

Steps To Reproduce

Feature::activate('custom-font','Agu Display');
Feature::deactivate('custom-font');

[timacdonald]

Interesting idea! I'm not against the idea, depending on maintenance burden of the feature.

Due to the existing behaviour and expectations of developers using the package already, I imagine we would need a dedicated API for this, e.g.,

Feature::activate('custom-font', 'Agu Display');

Feature::deactivate('custom-font');                 // remembers 'Agu Display' somehow

Feature::restore('custom-font', fallback: 'Arial'); // will restore to 'Agu Display'

This seems pretty reasonable to me and feels like a good fit for the project; features will often be deactivated, when used for feature rollouts, if there is a bug. Having the ability to restore the feature to the previous state for the user feels like a valuable feature.

[timacdonald]

@inmanturbo, would you want to have a first go at the PR and I can take a look at it? If not, I'll keep this one up my sleeve for when I have some time to enhance the package.

[inmanturbo]

@timacdonald Thank you for taking look at the idea and sharing your thoughts! I agree a separate api makes sense, and Feature::restore does seem the most natural fit. fallback is a nice touch.

I don't mind taking a stab at it. And I would indeed have use for it in the package. I don't know how soon I'll be able to pick it up however, as much as I'd love to start on it right away! Hopefully soon 😄

[timacdonald]

No stress. No expectations that you would have time. I just didn't want to step on your toes if you were keen to make it happen.

Will close this off because it isn't an issue with the existing feature set.

Will try to circle back to it soon.

[inmanturbo]

@timacdonald What do you think about putting the act of a non destructive or temporarily disabling of a rich feature behind a separate API as well? Something like:

Feature::activate('custom-font', 'Agu Display');

// Pause for bug fix, remembers 'Agu Display'
Feature::pause('custom-font');

// Resume using the feature with paused values
Feature::resume('custom-font', fallback: 'Arial'); // 'Agu Display'

// Behaves as it always has, forgets 'Agu Display'
Feature::deactivate('custom-font'); // false

// Will use fallback, as 'Agu Display' is forgotten
Feature::resume('custom-font', fallback: 'Arial'); // 'Arial'

This may make it simpler to rollout and maintain without breaking any existing API.

[timacdonald]

Because this feature will require a new database column, my feeling is it is already breaking so we can make the best decisions around the API.

I like the simplicity of exposing the Feature::restore method and having everything Just Work™️.

Feels like less decisions in a likely stressful time, e.g., "make sure to call pause instead of deactivate, and feels like magical in the best way.

What do you think about that?

[inmanturbo]

@timacdonald Yes I agree, if we are accepting or open to adding a migration. I was thinking about using pause as part of an alternative or workaround for that. Perhaps adding logic to the driver for how values are stored when calling pause, without modifying the way they are stored now, when calling deactivate. Adding the column is nicer though, I think. Better to do something "All the way" and have it Just Work like magic I suppose, than to have a patchwork of workarounds with a whole new API.

The single value column is already doing extra work.

[inmanturbo]

I guess some of the inspiration for considering something like pause comes from the way one grows accustomed to adding macros when additional functionality is needed.

As for adding the column. My primary concern is for how existing features should be handled. I don't think we would want a migration which modifies existing records, nor does it seem ideal to have a step in the upgrade guide which requires running php artisan pennant:purge.

[inmanturbo]

@timacdonald I've started a WIP here: #135

Please take a peek when if you get a chance and let me know I'm on the right track.