SS4 caching: update Zend_Cache or PSR-6? #6252
Comments
|
I think we should switch to using PSR-6 for this, and then providing a default PSR-6 library as a framework dependency. This is what we did with PSR-3 / monolog, too. I think that we should refactor SS_Cache to accept any PSR-6 implementation and ditch the Zend_Cache support. |
|
This library seems to be one of the more popular PSR-6 implementations: |
sminnee
changed the title
|
IIRC, one of the goals of simplecache was that it adds a thin API layer on top of PSR-6 to simplify it. Whether that’s something that the library would provide, I’m not sure - but I don’t think the two standards are mutually exclusive/competing. +1 for PSR-6. |
|
Another implementation is symfony/cache, but stash seems to be more popular (https://packagist.org/providers/psr/cache-implementation) |
|
👍 for simplecache over psr-6 |
|
SimpleCache is a layer on top of PSR-6, that doesn't current have very widespread implementations. So "SimpleCache over PSR-6" doesn't really make sense. Using SimpleCache means using PSR-6. That said, I think that SS classes directly accessing PSR-6 is a bit overblown. There needs to be some layer between the two, and we have a few choices for that:
|
|
This is the only SimpleCache implementation, apparently: http://www.scrapbook.cash/. It provides it as part of a caching library. At 20,000 Packagist downloads it's less popular than Stash and Symfony cache. |
|
Surely the point of using a PSR-6 caching lib is that we can use |
|
If there are no SimpleCache implementations yet, my vote goes to refactoring |
|
So, what it sounds like the implication of what Loz is suggesting is:
To be honest I would probably split out our simplecache implementation as What's not decided yet is how we define the caches. We could provide injected services such as In either case, it also begs the question of whether we need |
|
For reference, these are the caches that we use in core:
|
|
Just to confirm: the new
My preference is the latter - my understanding of the first option is that we couldn’t then (easily) set different configuration options for different uses of the same backend. Let’s say I want to override core caches to cache everything with
I think it could still be useful as a convenience layer. We could use $cache = SilverStripe\Cache\Cache::factory('Apc', ['ttl' => 600]);
// vs...
$cache = SilverStripe\Core\Injector\Injector::inst()->get('SilverStripe\Core\Cache.Apc');
$cache->getDriver()->setOptions(['ttl' => 600]);There’s not a huge difference there, but it should be a pretty thin API layer to maintain for the added convenience. |
If we provided a configuration API then I would say so, but my preference would be that we just pass responsibility for configuration over to the 3rd part API and don't attempt to build any kind of configuration adapter at all — I don't think it would be a value-adding layer.
I don't think the TTL argument here is appropriate. I think that the TTL setting should be part of the service definition for the APC cache. |
Looks like it’s a SimpleCache implementation specifically for scrapbook, but not a PSR-6 “wrapper” as such. PSR-16/SimpleCache is under review at the moment, and these messages ([1] and [2]) seem to suggest that a PSR-6 to PSR-16 bridge will live in a |
|
This might be a good time to come up with a solution for the cache cross-pollination issue raised by Nicolaas - we basically need a way for the system to influence the cache key, e.g. by adding a domain. |
|
Having spent a bit of time working with caches in the last few days, but NOT having read all the stuff above or understanding it all ... Here are a few things I have thought of:
Sorry for the long rant. I hope it helps in getting a better caching system soon ;-) |
|
@sunnysideup I appreciate you’ve had a few frustrating issues with caching recently, thanks for your work in tracking them and their causes down. We might not be able to fix them all, or fix them cleanly, in the 3.x releases but it’s helpful to know what problems we need to avoid in future. I won’t touch on all of your points above, but we’ll definitely avoid writing our own caching system from scratch. Caching is deceptively hard to get right, and there are many mature, well-tested libraries out there that handle it very well ( |
|
@kinglozzer - thank you for your reply. Totally agree with the maxim of using something existing and that being better than homebaked. Having said that, you should consider this: because the current caching system is hard to understand and tries to allow for so many options, with different settings, etc... it basically meant that SS 3 did not have a proper caching system beyond file caching. In other words, you may move the grunt work of the caching to a third-party system, but if that third-party system is complex and has many options then you may end up writing a larger amount of code to deal with the options then the amount of code required to write our own solution. Thus, I would advocate for something really fast and simple with the ability to switch it out for something more complex. |
|
So it sounds like the main point of contention is retaining Looking at the SS_Cache API, we have the following methods:
Just to clarify, is anybody suggesting to create a "PSR-6/PSR-16 to Zend_Cache" adapter, to avoid any existing cache users changing their code? Or is this just about retaining Sorry, the wording of the options ("internally") implied that we weren't requiring devs to change their own code use, and retain the |
|
Ingo I think that level of detail is best addressed at the PR stage. Might be best to start coding? |
|
I'm becoming less attached to the idea of keeping If so, I'm happy to go with the crowd on this one |
|
The reason I'd be against keeping |
I was imagining something like: $partialCache = Injector::inst()->get('Psr\Cache\CacheItemPoolInterface.Partials');
$item = $partialCache->getItem('foo');
if($item->isHit()) {
return $item->get();
} else {
$value = doSomething();
$item->set($value);
return $value;
}and your yaml Instead of the base service name |
|
Status update: I've started implementing this and hitting some conceptual road blocks. I've identified a few implied requirements from the 3.x-style API:
In Symfony's PSR-16 cache drivers, this is achieved by Every cache has it's own namespace in order to be clearable without affecting other caches ( Namespace option 1: Create lightweight driver-specific factories return PSR-16 objects, e.g. Namespace option 2: Use StashPHP + PSR-6, which means we can use Pool->setNamespace() and consistently rely on the SS Injector with a minimal Namespace option 3: Drop the "Allow creating a new named cache without writing a bunch of boilerplate YAML config" requirement. This would mean any project would need to know about all cache implementations used throughout core, modules and project code, and duplicate the same YAML config block for each of them to consistently set a different driver (e.g. exchanging filesystem for memcache). |
|
I think that option 1 is the best road to go down. I'd define a CacheFactory interface for these factory objects, and have all the default cache services use something like this in their service definition CacheFactory.php namespace SilverStripe\Core\Cache;
interface CacheFactory
{
function create($namespace, $lifetime = null);
}cache.yml Encourage module developers to do the same.This will mean that the CacheFactory service can be redefined and all default caches will be replaced. |
|
It's your call as to whether the cache factory interface needs a setLifetime as sell |
|
@chillu I amended my example above after looking at how Injector actually works. ;-) Principle is the same |
|
Oh the other thing you could do is have a generic cache factory that lets you define the argument order: Or With code something like this: class DefaultCacheFactory implements CacheFactory {
public $Class, $Constructor;
function create($namespace, $lifetime) {
$mappings = [ '$namespace' => $namespace, '$lifetime' => $lifetime ];
foreach($this->Constructor as $arg) {
$mappedArgs[] = isset($mappings[$arg]) ? $mappings[$arg] : $ar;g
}
return Injector::inst()->create($this->Class, $mappedArgs);
} |
Replaced with a PSR-16 implementation
|
Pull requests: #6648 and silverstripe/silverstripe-cms#1741 |
Replaced with a PSR-16 implementation
Replaced with a PSR-16 implementation
UPDATE: Ticket description has been extended/replaced by @chillu on 10th of Feb
Tasks
Goals
Scope
Usage Comparison
Libraries
psr/cache,psr/log,psr/simple-cachepsr/cachepsr/cache,psr/simple-cacheOptions
Option 1a: Replace SS_Cache with a PSR-6 library
lock()in stashphp)Option 1b: Adapt SS_Cache to use a PSR-6 library internally
Option 2a: Replace SS_Cache with a PSR-16 library ("simplecache")
Option 2b: Adapt SS_Cache with a PSR-16 library internally
Option 2c: Refactor SS_Cache to become a PSR-16 library
Configuration
The majority of cached should be configured through YAML
(apart from the class and config manifest caches which need to be in place before YAML can be parsed).
This configuration leverages the SilverStripe dependency injection system
for configuring instances, alongside service names like
Cache.driver.memcached.The configuration assumes PSR-6 (referring to "pools"), but could be adapted to PSR-16.
Before:
_config.php
After:
config.yml
Usage for class/config manifest (before YAML is available):
Core.php
boot-cache.php
Note: Additional boot files could be handled in a more generic fashion,
this just aims to illustrate the principle (inject procedural code early during boot).
It's going to be different to handle this via environment variables alone,
since different drivers have different construtor arguments, each of which
would need to be reflected in environment variables.
Pool Management
In Option 1a and 2a, SilverStripe developers are expected to directly interact with
a PSR-6 or PSR-16 library.
Before:
After (with PSR-6):
If different drivers ('frontends') could be set on the cache pool instance,
a direct choice via the
factory()should be avoided.Most use cases should be covered by configuring drivers and driver
options per cache pool definition, not per cache pool instance.
Notes
CacheItemInterface->expiresAfter()) and PSR-16 (CacheInterface->function set($key, $value, $ttl = null)). If APIs likei18nwant to expose one global value, they should make it configurable in their own namespace (e.g.i18n.cache_ttl)like logging cache hits, or providing structured data about cache access to a developer toolbar. StashPHP has a
setLogger()methodFurther reading
Related Issues
The text was updated successfully, but these errors were encountered: