fix: catch cache error when cache is unavailable - #8
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds error handling to gracefully handle cache unavailability scenarios, such as when the cache database doesn't exist during application boot. The changes prevent exceptions from being thrown when cache operations fail, allowing the application to continue functioning without cache.
Changes:
- Added try-catch blocks around cache operations in
active(),forgetCache(), andflushCache()methods - Import
Throwableclass for exception handling - Added comprehensive test coverage for cache unavailability scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ToggleManager.php | Added exception handling to cache operations to prevent failures when cache is unavailable |
| tests/Feature/CacheTest.php | Added three test cases covering cache unavailability during active check, forgetCache, and flushCache operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (Throwable) { | ||
| // Cache unavailable | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Catching all Throwable instances is too broad. This catch block could also suppress errors from getDriver()->all() which is unrelated to cache operations. Consider catching more specific exceptions related to cache failures, or restructuring to only wrap the cache-specific operations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| return $this->getCache()->forget($this->cacheKey($key)); |
There was a problem hiding this comment.
The forgetCache() method wraps getCache() in a try-catch, but getCache() is also called in the enable(), disable(), and delete() methods at lines 69, 84, and 99 without error handling. If cache is unavailable when these methods are called, they will throw an exception. Consider adding similar error handling to these call sites or ensuring forgetCache() can be called safely in all contexts.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Cache::shouldReceive('store') | ||
| ->andThrow($exception); |
There was a problem hiding this comment.
The mock setup using Cache::shouldReceive('store') is incorrect. The code calls forget on the cache repository, not the store method. The mock should call shouldReceive('forget') instead, or mock the entire cache store chain properly to actually trigger exceptions during the forget operation.
| Cache::shouldReceive('store') | ||
| ->andThrow($exception); |
There was a problem hiding this comment.
The mock setup using Cache::shouldReceive('store') is incorrect. The code calls forget on the cache repository in a loop, not the store method. The mock should call shouldReceive('forget') instead, or mock the entire cache store chain properly to actually trigger exceptions during the forget operations.
| try { | ||
| $cache = $this->getCache(); | ||
|
|
||
| // Clear all known toggles from cache | ||
| foreach ($this->getDriver()->all() as $name => $value) { | ||
| $cache->forget($this->cacheKey($name)); | ||
| } | ||
| // Clear all known toggles from cache | ||
| foreach ($this->getDriver()->all() as $name => $value) { | ||
| $cache->forget($this->cacheKey($name)); | ||
| } | ||
|
|
||
| return true; | ||
| return true; | ||
| } catch (Throwable) { | ||
| // Cache unavailable | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The try-catch block around the entire flushCache implementation catches all Throwable exceptions, including those that might be thrown by getDriver()->all(). This could mask genuine driver errors that are unrelated to cache unavailability. Consider wrapping only the cache-specific operations (getCache() and cache->forget()) in the try-catch block, or explicitly checking the exception type before swallowing it.
|
|
||
| it('gracefully handles cache unavailability during active check', function () { | ||
| // Mock cache to throw a QueryException (simulating database not existing) | ||
| $exception = new QueryException('sqlite', 'select * from cache', [], new Exception('Database does not exist')); |
There was a problem hiding this comment.
The Exception class is used without a proper import statement or full namespace qualifier. Add use Exception; to the imports at the top of the file, or use \Exception in the code.
| }); | ||
|
|
||
| it('gracefully handles cache unavailability during forgetCache', function () { | ||
| $exception = new QueryException('sqlite', 'select * from cache', [], new Exception('Database does not exist')); |
There was a problem hiding this comment.
The Exception class is used without a proper import statement or full namespace qualifier. Add use Exception; to the imports at the top of the file, or use \Exception in the code.
| }); | ||
|
|
||
| it('gracefully handles cache unavailability during flushCache', function () { | ||
| $exception = new QueryException('sqlite', 'select * from cache', [], new Exception('Database does not exist')); |
There was a problem hiding this comment.
The Exception class is used without a proper import statement or full namespace qualifier. Add use Exception; to the imports at the top of the file, or use \Exception in the code.
| Cache::shouldReceive('store') | ||
| ->andThrow($exception); |
There was a problem hiding this comment.
The mock setup using Cache::shouldReceive('store') is incorrect. The code calls methods like remember, forget on the cache repository, not the store method. The mock should call methods like shouldReceive('remember') or shouldReceive('forget') instead, or mock the entire cache store chain properly to actually trigger exceptions during these operations.
No description provided.