[1.x] Allows easy toggling of Feature's value

[inmanturbo]

Hey all

This Pr adds a helper for toggling features

Adds the following method to PendingScopedFeatureInteraction.php

    /**
     * Toggle the feature's value.
     *
     * @param  string|array<string>  $feature
     * @param  mixed  $value
     * @return void
     */
    public function toggle($feature, $value = null)
    {
        Collection::wrap($feature)
            ->crossJoin($this->scope())
            ->each(fn ($bits) => $this->driver->set($bits[0], $bits[1], match (true) {
                null === $value => false === $this->driver->get($bits[0], $bits[1]) ? true : false,
                false === $this->driver->get($bits[0], $bits[1]) => $value,
                default => false,
            }));
    }

shortens this:

Feature::for(auth()->user())->active('new-design')  
    ? Feature::for(auth()->user())->deactivate('new-design')
    : Feature::for(auth()->user())->activate('new-design');

to this:

Feature::for(auth()->user())->toggle('new-design');

EDIT:

Also supports passing a value, in order to toggle inactive and unresolved feature(s) to active with a rich value.

Feature::for('scope')->toggle('danger-button', 'bg-red-200');

[inmanturbo]

@timacdonald I see you've marked this as draft. Should I leave it as draft for now then?

[timacdonald]

@inmanturbo, if you could for now, yea. Not against the feature, just think we might have to incorporate a "value" to toggle it too, as values aren't always true / false.

Just might not get to look at this right away with the Laravel 11 release this week.

[inmanturbo]

@timacdonald thanks for pointing that out. I forgot the about the rich feature options!

[inmanturbo]

@timacdonald thanks for the tests!

[timacdonald]

No troubles. I think there is still some tweaks here to be made. I'd like to take another crack at this shortly. Just been thinking on if we need to introduce some more dedicated method on the drivers for this.

I might make this one a draft again, otherwise Taylor will review it and I'm not sure it is ready for his eyes yet.

[inmanturbo]

Yes, good call. I've realized there needed to be a check to be sure whether the bits being evaluated are bool, because for instance ! '0' evaluates to true.

[inmanturbo]

@timacdonald I've updated the snippets in the Initial PR post to better reflect the current 'gist'. I'm going to leave it be for now, and leave it as a draft.

[timacdonald]

Been thinking on this. I think my only concern here is race conditions where you retrieve the value, but another process changes the value before it is "set" again.

Do you have any thoughts on that?

[inmanturbo]

I think the "other" process in this case could possibly "perceive" an unexpected outcome. The toggle would work as expected I believe, because it would still set the value opposite to what it was at the time of retrieval.

The same risk is run with the following snippet

Feature::for('scope')->active('new-design')  // true
    // another process runs: 
    // Feature::for('scope')->deactivate('new-design')
    ? Feature::for('scope')->deactivate('new-design')
    : Feature::for('scope')->activate('new-design');

The above produces the same result regardless, however

Feature::for('scope')->active('new-design')  // false
    ? Feature::for('scope')->deactivate('new-design') // skipped
    // another process runs: 
    // Feature::for('scope')->activate('new-design', 'dracula')
    : Feature::for('scope')->activate('new-design', 'materialize-light'); 

toggle 'wins'

To be fair, the above snippet and any of its (possibly unexpected) outcomes wouldn't have to be maintained by Pennant. And just as disclaimer I did tinker a little bit while writing this but I'm not 100% sure none of the above would result in an error.

If you think there could be the possibility of a problem with atomic locks or a crash, such as if toggle were to find itself attempting to modify a row that is no longer there, or the row is locked by another process, it may be worth moving the feature into the driver where there it could be better mitigated using driver features: transactions, etc or whatever is available to a particular driver. Or possibly even avoid adopting this feature altogether.

As for the database driver I might explore that a bit myself when I have more time.

[inmanturbo]

I'm going to try this feature out on my own for awhile. If everything goes well I'll reopen later.