-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Conversation
|
||
namespace Zend\Cache\Storage\Adapter; | ||
|
||
use Memcache as MemcacheResource; |
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.
unused use Memcache
Thanks for the review, @samsonasik. I think I've gotten everything buttoned up now. Let me know if I've missed anything. |
👍 |
* @return array Array of not stored keys | ||
* @throws Exception\ExceptionInterface | ||
*/ | ||
protected function internalSetItems(array & $normalizedKeyValuePairs) |
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 be dropped as the memcache
extension doesn't support setting items as bulk and setting it as a loop should be done already by the abstract adapter.
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.
Thanks. I think I was going cross-eyed from comparing the Memcache/Memcached documentation and was looking at the wrong setMulti
page at the time. Will fix.
Only the above comments found. |
…et multiple value internal method
Thanks @marc-mabe for the review. I think I've got all of your comments addressed, except for the one about booleans that I've replied to. Let me know if there's anything else I've missed. Thanks again. |
Finished testing differences between ext/memcache 3.x beta and 2.x stable versions, which affects the adapter capabilities: https://github.com/cgmartin/zf2/blob/2c1649e9ca8b7683cd10ad0e65e178a60af49cf5/library/Zend/Cache/Storage/Adapter/Memcache.php#L481-517 |
I had commented on the method |
* @param MemcacheResource $resource | ||
* @param array $libOptions | ||
*/ | ||
protected function setResourceLibOptions(MemcacheResource $resource, array $libOptions) |
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.
The lib_options
was in the memcached adapter to proxy to Memcached::setOption
which changes from version to version. In my opinion it isn't needed for ext/memcache
and it would be better to make it a normal getter/setter [get|set]AutoCompressThreshold
and [get|set]AutoCompressMinSaving
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, I can empathize with that. My original thought was to try make it similar to the Memcached
adapter config, but at second glance it just seems awkward here. Will do.
@marc-mabe regarding the
|
Zend\Cache\Storage\Adapter\Memcache
- Promoted contents of several conditional statements, so that the bulk of the methods' work does not happen inside conditionals.
Merged to develop for release with 2.3.0. |
…cache-adapter Zend\Cache\Storage\Adapter\Memcache
- Promoted contents of several conditional statements, so that the bulk of the methods' work does not happen inside conditionals.
New cache storage adapter for ext/memcache (not to be confused with existing ext/memcached adapter).
Implements feature request: #4481
An example configuration for
Zend\Cache\StorageFactory
would be: