Allow Feature values to be restored

[inmanturbo]

fixes #133

WIP

Todo:

• Add database driver tests for Feature:restore
• Implement Feature::restore()
• Implement Feature::restoreForAll()
• Test fallback behavior

[github-actions]

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.

[inmanturbo]

@timacdonald I just started this to see if it may knock loose some inspiration for later, and may not get back to it for a bit. Please feel free to disregard my toes altogether 😄 . If you have an idea for an implementation and get a chance to work on it, go ahead and make changes here or disregard this whole thing and render it obsolete at will!

[inmanturbo]

@timacdonald Leaving as draft until you've had a chance to take a look, there may be some more work still to do here. I've introduced a Laravel\Pennant\Contracts\Driver::getRaw() method for retrieving values regardless of activation. I've avoided a fallback in Feature::restoreForAll() as that would require checking each row anyway. What do you think?

[timacdonald]

Appreciate it. I'll certainly come and check this out and see if we can push it forward.

[inmanturbo]

No worries. Please feel free to make or request changes, or scrap in favor of a different approach.

Some things I'm admittedly not particularly fond of in the (my) current approach:

Decorator::restoreForEveryone()

    /**
     * Restore the feature for everyone.
     *
     * @param  string|array<string>  $feature
     * @return void
     */
    public function restoreForEveryone($feature)
    {
        Collection::wrap($feature)
            ->each(fn ($name) => $this->setForAllScopes($name, null));
    }

I threw it in for symmetry, but it may not be worth the trouble.

• Using setForAllScopes in this way, passing in null and switching on it feels a little odd.
• getRaw() is required for restoring for a single scope, but it isn't called here so there is no check in place during restoreForEveryone to tell whether a driver is supporting the new restore feature.

ArrayDriver::getRaw()

    /**
     * Retrieve a feature flag's raw value.
     *
     * @param  string  $feature
     * @param  mixed  $scope
     * 
     * @return mixed
     */
    public function getRaw($feature, $scope)
    {
        throw new RuntimeException('This pennant driver does not support getting raw values.');
    }

The array driver just throws an exception when calling getRaw. There is precedence in the ecosystem, of course. For instance the abstract Illuminate\Database\Schema\Grammars class in the illuminate/database package will throw an exception if you attempt to call compileSchemas using a driver that does not support it.

It does provide an example of what driver maintainer can do when upgrading, if their driver does not, or cannot, support the restore feature.

But I think it could be kinda nice to just implement the feature in the array driver by adding another key to the array, as both an example for how drivers for key value stores might be implemented, and a for cheap, full featured driver to use during testing. Such a thing could lead to confusion however, as obviously it cannot persist accross application boots.

Laravel\Pennant\Contracts\Driver::getRaw()

If we are preserving the values even for inactive features, it of course makes sense to have a way to inspect those values, but perhaps a there is a better name for this method. And should there be a getAllRaw? It might be useful to improve restoreForEveryone. I just don't know where we want to draw the line on this feature.