Custom setting store doesn't use cache (or incorrect instructions)

[bjhijmans]

I had to extend the DatabaseSettingStore for some unimportant reasons. So I followed the instructions in the readme and added the a new class:

class CustomDatabaseSettingStore extends Base
{
    public function __construct(Connection $connection)
    {
        parent::__construct(
            $connection,
            config('settings.table'),
            config('settings.keyColumn'),
            config('settings.valueColumn')
        );
    }
    // more code here
}

and I added the following to my AppServiceProvider:

Setting::extend('customDatabaseSettingStore', function ($app) {
    return $app->make(CustomDatabaseSettingStore::class);
});

I have caching enabled, but I found the setting cache suspiciously empty. So I checked, and it turns out that caching settings are never set in the custom store, and I think defaults also don't work. These are usually set in SettingsManager::wrapDriver() which is never called.

I had to add the code from wrapDriver() to my custom store:

class CustomDatabaseSettingStore extends Base
{
    public function __construct(Connection $connection)
    {
        parent::__construct(
            $connection,
            config('settings.table'),
            config('settings.keyColumn'),
            config('settings.valueColumn')
        );

        $this->setDefaults(config('settings.defaults'));

        if (config('settings.enableCache')) {
            $this->setCache(
                app()['cache'],
                config('settings.cacheTtl'),
                config('settings.forgetCacheByWrite')
            );
        }
    }
// more code
}

I think, at the very least, the readme should be updated. I was very surprised that this was needed, especially because it is usually handled by the SettingsManager, which I wouldn't have expected to have to look at. I would prefer if wrapDriver() was somehow called for custom drivers, but it would already be helpful if it were public and easily accessible.

[anlutro]

Hello! This makes total sense. Laravel's Manager class doesn't provide convenient hooks to do the kind of things I wanted with wrapDriver but I think this is achievable with #169.

Do you want to pull the version dev@fix-wrapdriver-custom-store to see if that fixes your issue before I merge?

[bjhijmans]

Hey! That looks good. I can check it out when I'm back at work on Monday.

I'm not sure about your decision to make it a separate class, though. There's already a setting to disable the cache. I don't see the added value to using a separate class AND a config setting to enable it.

[anlutro]

The extra class is two-fold: It's a way to detect whether the store should support caching (the in-memory one doesn't make sense, for example), and it's to be backwards compatible - the current custom stores don't support caching (as you pointed out) so now this is opt-in behavior with a new class.

I'm not entirely convinced this is the correct approach, especially not having worked with Laravel for more than 5 years now, but it feels right.

[bjhijmans]

I checked it out. There's a bug currently, readData() is private in both SettingStore and CachedSettingStore, but it is called from SettingStore, so it calls that version and skips caching. It works fine if they're both protected.