-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
Removed unnecessary private method from CacheManager
#26048
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe private Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/shared/settings-cache/cache-manager.js (1)
134-149: Guard override mutation when the cache entry is missing.
cacheEntryis mutated before the null check. IfsettingsOverridescontains a key not present insettingsCache, this will throw. Consider moving the null check earlier (or creating a stub entry) to avoid a runtime crash.🛠️ Proposed fix (minimal guard)
- const cacheEntry = this.settingsCache.get(key); - - if (override) { - cacheEntry.value = override.value; - cacheEntry.is_read_only = true; - } - - if (!cacheEntry) { - return; - } + const cacheEntry = this.settingsCache.get(key); + if (!cacheEntry) { + return; + } + if (override) { + cacheEntry.value = override.value; + cacheEntry.is_read_only = true; + }
no ref This change should have no user impact. `CacheManager.prototype.get` is just an alias for `_doGet`. Let's remove `_doGet` and just use the public method.
3ce24d9 to
8c9bc98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/shared/settings-cache/cache-manager.js (1)
140-149:⚠️ Potential issue | 🟡 MinorReorder the null check before override modification to prevent potential TypeError.
Lines 140-149 access
cacheEntry.valuebefore checking ifcacheEntryis null. If a key exists insettingsOverridesbut not insettingsCache, this will throw a TypeError. The null check must come immediately after retrieving the cache entry.🛡️ Suggested fix: reorder the null check
const cacheEntry = this.settingsCache.get(key); + if (!cacheEntry) { + return; + } + if (override) { cacheEntry.value = override.value; cacheEntry.is_read_only = true; } - - if (!cacheEntry) { - return; - }
no ref
This change should have no user impact.
CacheManager.prototype.getis just an alias for_doGet. Let's remove_doGetand just use the public method.Summary by CodeRabbit