Skip to content
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

[Feature Req] Cache settings #90

Open
rk opened this issue Dec 10, 2019 · 6 comments
Open

[Feature Req] Cache settings #90

rk opened this issue Dec 10, 2019 · 6 comments
Labels

Comments

@rk
Copy link

rk commented Dec 10, 2019

So, the current implementation of the SettingsServiceProvider::boot() method prevents caching of the settings, forcing 2 queries (one to detect the table, one to load settings) on the DB. It's more efficient for larger sites to cache all settings in memcached or redis, instead of querying the DB on every page hit.

I looked into extending the provider, but that might be a bit messy.

If you define a static method for it on the Setting model, then using a config value to point at the FQCN. We could then override the Setting class and extend it to allow caching.

Just some food for thought.

@welcome
Copy link

welcome bot commented Dec 10, 2019

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication mediums:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

@tabacitu
Copy link
Member

Hmm you're right @rk . Thank you for the suggestion. Since settings are changed so rarely, it makes a lot of sense for them to get cached. We just need to make sure that once a Setting is Updated, we wipe the cache - so that changing settings would still have immediate effect.

I see there's already a PR for this here #91
Let me know what you think of it.

Cheers!

@rk
Copy link
Author

rk commented Dec 16, 2019

It does need the logic for clearing once a setting is updated, but it looks appropriate. 👍 It caches both the table check and the resulting fields separately, which is good.

The event is really simple to add though:

Setting::saved(function () {
    Cache::forget('backpack_settings_cache');
});

@tabacitu
Copy link
Member

Fixed by #91 . I'll take another look at it before we launch Backpack 4.1 (this or next week), merge it and tag it as a new version. Let's move the conversation there please.

@eduardoarandah
Copy link
Contributor

I'd like to provide some feedback on this.

I have a project where I have to cache one route because of the high traffic it receives.

The route was cached via redis, so it shouldn't touch the database at all.

However, this package still made queries to database, bringing performance down significantly.

Here's some ideas on how to get cache config and use it:
https://github.com/spatie/laravel-permission/blob/master/src/PermissionRegistrar.php

@marechenok
Copy link

In my situatuion we've moved DB server to remote host and now for each http request it produces 2 requests to DB. Even if this pages uses none of settings. We have a lot of settings and all data generates certain amount of traffic. Wich is also paid.

Another suggestion, when loading settings in ServiceProvider:
Don't load everything from DB like that:

Setting::all();

Just take key and value - for most cases it covers 99% of usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants