Open bjhijmans opened 1 year ago
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 https://github.com/anlutro/laravel-settings/pull/169.
Do you want to pull the version dev@fix-wrapdriver-custom-store
to see if that fixes your issue before I merge?
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.
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.
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.
I had to extend the DatabaseSettingStore for some unimportant reasons. So I followed the instructions in the readme and added the a new class:
and I added the following to my AppServiceProvider:
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: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.