-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom setting store doesn't use cache (or incorrect instructions) #168
Comments
Hello! This makes total sense. Laravel's Manager class doesn't provide convenient hooks to do the kind of things I wanted with Do you want to pull the version |
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, |
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.The text was updated successfully, but these errors were encountered: