Add a prototype for the file system cache #551
Conversation
Hectorhammett
added
the
do not merge
| if (is_dir($this->cachePath)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This seems like a blunder : )
Do you want to do something like, "throw exception of the cachepath isn't valid" ?
There was a problem hiding this comment.
Oh my. Yeah haha I basically want to handle the case where the cache folder already exists. But returning true from a __construct is definitely a boo boo haha.
In that topic, what do we think it would be the best option when a cache folder already exists? Just re use it? Raise an exception? Clear it?
| } | ||
|
|
||
| if (!mkdir($this->cachePath)) { | ||
| throw new ErrorException("Cache folde couldn't be created"); |
There was a problem hiding this comment.
| throw new ErrorException("Cache folde couldn't be created"); | |
| throw new ErrorException("Cache folder couldn't be created"); |
| if ($this->isValidKey($key)){ | ||
| throw new InvalidArgumentException("The key '$key' is not valid. The key should follow the pattern |^[a-zA-Z0-9_\.! ]+$|"); | ||
| } | ||
|
|
There was a problem hiding this comment.
We're already checking for valid key in the getItem method so we can remove this here
| if ($this->isValidKey($key)){ | |
| throw new InvalidArgumentException("The key '$key' is not valid. The key should follow the pattern |^[a-zA-Z0-9_\.! ]+$|"); | |
| } |
There was a problem hiding this comment.
True! Good catch!
| return false; | ||
| } | ||
|
|
||
| $serializedItem = file_get_contents($this->cacheFilePath($key)); |
There was a problem hiding this comment.
We can reuse the calculated path.
| $serializedItem = file_get_contents($this->cacheFilePath($key)); | |
| $serializedItem = file_get_contents($itemPath); |
| /** | ||
| * {@inheritdoc} | ||
| */ |
There was a problem hiding this comment.
I know this reduces the documentation but at the cost of affecting readability in an non ide / intellisense environment.
I feel having at least a one-line here would help understand what's expected of a particular method.
There was a problem hiding this comment.
Yeah absolutely.
We should add more documentation on the final release.
| return false; | ||
| } | ||
|
|
||
| foreach (scandir($this->cachePath) as $fileName) { |
There was a problem hiding this comment.
Directly iterating upon scandir may break the code as it can return false on any type of failure (for eg, inadequate access permissions). Are we sure we'll never encounter permission errors?
If the intention is to break naturally with errors, then I feel we should handler the errors gracefully with error message explaining what's happening. WDYT?
There was a problem hiding this comment.
Absolutely.
I wonder if we should utilize the error_reporting function as the psr6 spec does not specifies this function throwing exceptions.
Besides another characteristic of caching is the fact that the cache can fail and the program should not stop working, so my thought is using some sort of logging to report the problem.
But we definitely need to catch specific errors.
| if (!$this->isValidKey($key)) { | ||
| throw new InvalidArgumentException("The key '$key' is not valid. The key should follow the pattern |^[a-zA-Z0-9_\.! ]+$|"); | ||
| } | ||
|
|
There was a problem hiding this comment.
deleteItem method is already doing this check.
| if (!$this->isValidKey($key)) { | |
| throw new InvalidArgumentException("The key '$key' is not valid. The key should follow the pattern |^[a-zA-Z0-9_\.! ]+$|"); | |
| } |
| return $result; | ||
| } | ||
|
|
||
| public function saveDeferred(CacheItemInterface $item): bool |
There was a problem hiding this comment.
This method could cause issues hogging too many resources as every pool will use their own implementation for deferring an item save.
An option would be to implement a buffer on this class and call save() on each buffer on commit() to avoid exploding the resources.
| } | ||
| } | ||
|
|
||
| return $result; |
There was a problem hiding this comment.
I am using the description for psr6 where they mentioned on these bool results to return false if there is any error.
This means that the user of this CacheChain will be receiving false when we cleared or deleted items on some caches and not all of them.
I wonder if this would create a bug/issue.
This is a prototype for #498