Camp 881 redis sets #10
Conversation
Lib/Cache/Cache.php
Outdated
| * | ||
| * @package Cake.Cache | ||
| */ | ||
| class Cache { |
There was a problem hiding this comment.
@jec3 Could you just extend the Cache class so you only have to override the write() method instead of having to copy the entire class?
There was a problem hiding this comment.
@jec3 Could you just extend the Cache class so you only have to override the
write()method instead of having to copy the entire class?
@dchancogne I want all of the existing uses of the Cache class to use write override. If we extend Cache we'll have to update all existing uses of the Cache class to use our new class. This seems messier than overriding the class entirely.
There was a problem hiding this comment.
Makes sense and the header comment is clear.
Lib/Cache/Engine/FileEngine.php
Outdated
| * @param string|array $parentKey Unused. | ||
| * @return bool True if the data was successfully cached, false on failure | ||
| */ | ||
| public function write($key, $data, $duration, $parentKey = '') { |
There was a problem hiding this comment.
@jec3 So $parentKey is ignored here but you need to override the class and method to respect the new write() signature?
There was a problem hiding this comment.
Correct, because this implements the abstract class CacheEngine it must match the arguments specified there, which includes $parentKey.
There was a problem hiding this comment.
@jec3 Should work. Would be nice to find a cleaner way to do it that doesn't involve copying these core files. But it's all fairly limited in scope so worth a try to see if it helps performance.
|
|
||
| public function key($key) | ||
| { | ||
| return Cache::engine($this->activeCache)->key($key); |
There was a problem hiding this comment.
This is a fix for an existing bug. Without a key method defined here the definition in the abstract CacheEngine class is used, which modifies the key, unlike some other engines like the RedisTreeEngine, which uses the key exactly as provided.
| @@ -0,0 +1,12 @@ | |||
| FROM php:7.1-fpm-alpine | |||
There was a problem hiding this comment.
Cakephp 2.x requires mcrypt. PHP 7.1 is the last version that includes mcrypt so to avoid having to pull it from PECL I'm using 7.1 instead of 7.2.
| $finalKeys[] = $key; | ||
| $childKeys = $this->_getChildKeys($key); |
There was a problem hiding this comment.
To avoid searching all child key sets, no attempt is made to find other parents who share the same children. This means that there can be child key sets that include keys that no longer exist. This will cause an unnecessary increase in memory usage overtime but it should be kept in check by the fact that deletion of a key also deletes its associated child key set.
|
@dchancogne @rajivraman I did some reorganization based on a tip Elijah gave me. Can you give this one more pass? |
Looks good to me! |
Adds CacheEnginesHelper::writeWithParent.