outl1ne / nova-settings

A Laravel Nova tool for editing custom settings using native Nova fields.
MIT License
273 stars 97 forks source link

Using getValueForKey method instead of find in getSetting. #74

Closed phuclh closed 3 years ago

phuclh commented 3 years ago

By using getValueForKey method from the getSetting, it can be flexible to add a cache layer in the custom Setting Model by overriding getValueForKey method like this:

// CustomSettingModel.php

public static function getValueForKey($key)
{
    return Cache::rememberForever(self::getCacheKey($key), function () use ($key) {
        $setting = static::where('key', $key)->get()->first();

        return isset($setting) ? $setting->value : null;
    });
}
Tarpsvo commented 3 years ago

Thanks, that does make sense. :)

bluec commented 2 years ago

Thanks for this. Any clues about how to invalidate the cache when a setting is saved?

bluec commented 2 years ago

Answering my own question... I guess this would do it:

// CustomSettingModel.php

protected static function booted()
{
    static::updated(function ($setting) {
        Cache::forget(self::getCacheKey($setting->key));
    });
}
Tarpsvo commented 2 years ago

Heya! I added automatic memory cache clearing in version 4.0.4.

bluec commented 2 years ago

Hey @Tarpsvo thanks for this. I must admit I don't really understand the change and I wonder if we both have different meanings of "cache" in this context.

The shipped NovaSettingsStore class doesn't implement a true cache from what I can see. It only caches the settings within the currently instantiated object. So adding this on the booted() method of the model seems a bit confused and maybe also redundant. Every time the NovaSettingsStore class is instantiated it already has no cached values and when a setting is saved it already removes that setting from the cache.

This PR was about being able to use one of the Laravel cache backends for caching the settings values and reducing DB calls across multiple requests. The changes in this commit don't really solve this problem and actually seem quite confusing.

Tarpsvo commented 2 years ago

Yeah, it's an in-memory cache to avoid querying the database every time a value is requested during the same request. Ie if the user has multiple nova_get_setting('test') calls. This is just to help the user avoid duplicate SQL queries within the same request.

If you wish to cache the values into Redis or any other key-value store, you can overwrite the Settings model just like the original poster did.

bluec commented 2 years ago

Ok yes, so this means that the change in 4.0.4 to clean the cache using the booted() method on the Setting model doesn't make sense and doesn't actually do anything.

Tarpsvo commented 2 years ago

The observer clears the in-memory cache in case a setting is saved in-between nova_get_setting() calls. It both makes sense and actually does what it's supposed to. This package only offers in-memory caching out of the box.

If you want to cache your settings in a different cache layer (ie Redis), you have to overwrite the Settings model and the getValueForKey function just like in the original post. You also have to register an observer to clear the cache, either in an observer class or in the static::booted() function.

bluec commented 2 years ago

Ah I had assumed every call to nova_get_setting() instantiated a new NovaSettingsStore() object but I hadn't spotted that this class is registered in the service container as a singleton so this makes sense.

However, I still maintain that the observer is redundant since the in-memory cache is already cleared when a setting is saved and this would already clear the in-memory cache in case a setting is saved in-between nova_get_setting() calls.

(I understand about using a cache layer for more persistent cacheing which is what this PR was originally about.)