-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add a prototype for the file system cache #551
base: main
Are you sure you want to change the base?
Conversation
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.
Nice work @Hectorhammett
if (is_dir($this->cachePath)) { | ||
return true; | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! Good catch!
return false; | ||
} | ||
|
||
$serializedItem = file_get_contents($this->cacheFilePath($key)); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_\.! ]+$|"); | |
} |
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.
return $result; | ||
} | ||
|
||
public function saveDeferred(CacheItemInterface $item): bool |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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