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

The cache system in getSettings() cached values "forever." That's bad. #12326

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

uberbrady
Copy link
Collaborator

The Cache::rememberForever() invocation we were doing was way, way too aggressive - literally caching forever. Newly-updated Settings would end up not reflecting changes in the settings table until you could run php artisan cache:clear.

This changes that caching system to use a simple static property as an in-memory cache so that multiple invocations of Setting::getSettings() would only hit the disk once - per request. On the next request, it will hit the disk again - but, again, only once.

I tested this in Tinker multiple times by changing the database directly and repeatedly re-invoking getSettings() - which seemed to work as expected.

@what-the-diff
Copy link

what-the-diff bot commented Jan 10, 2023

  • The cache property was added to the Setting model
  • Cache::forget(Setting::APP_SETTINGS_KEY) is removed from saved() method in SettingObserver class

@@ -16,7 +16,6 @@ class SettingObserver
*/
public function saved(Setting $setting)
{
Cache::forget(Setting::APP_SETTINGS_KEY);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this forget the previous cache, therefore NOT remembering it forever?

@snipe
Copy link
Owner

snipe commented Jan 10, 2023

Newly-updated Settings would end up not reflecting changes in the settings table until you could run php artisan cache:clear.

That's not exactly true - anything made outside of a normal ->save() need to be cleared, since it would skip the SettingsObseverver's saved() method - which would only happen in a migration where we're adding a new column with a default value, no?

I'm not 100% convinced that this is the correct solution, but I'm willing to be convinced :)

@snipe snipe merged commit 20a9be2 into snipe:develop Jan 10, 2023
@uberbrady
Copy link
Collaborator Author

Well, I don't need to convince you anymore since you already merged it :P

@snipe
Copy link
Owner

snipe commented Jan 10, 2023

Not a hill I was willing to die on :P

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

Successfully merging this pull request may close these issues.

None yet

2 participants