FIX Ensure cache is shared between CLI and webserver #11300

GuySartorelli · 2024-07-04T02:27:47Z

Description

Removes default in-memory cache
Uses PhpFilesAdapter as the default filesystem cache adapter
Falls back on FilesystemAdapter only if PhpFilesAdapter isn't supported by both webserver and CLI
Adds new RedisCacheFactory as an option for in-memory cache
Adds new SS_MEMORY_CACHEFACTORY environment variable for opting in to in-memory cache
- Has to be an environment variable because config isn't available when the manifest cache adapter is instantiated
Amends ApcuCacheFactory and MemcachedCacheFactory to use args passed into create() or environment variables instead of constructor args
Use symfony to build Memcached client instead of relying on injector (injector config isn't available early enough)
Strict typehinting for Factory::create() and its subclasses
Unit testing that validates the correct cache adapters are used, and that their logger is set correctly

There is an edge-case drawback to this approach.
All redis/memcached cache will always use the same connection details. You couldn't for example use one redis server to cache some stuff, and another server to cache some other stuff.
The workaround for that would be to implement your own CacheFactory class for instantiating the cache adapter for the stuff you want on another server. So we're not locking out that possibility, just requiring a little bit more boilerplate if people want to do that.

Requires

The following PRs need to be merged for CI to go green:

Issues

Cache is not correctly shared between CLI and non-CLI #11145

GuySartorelli · 2024-07-04T02:39:18Z

src/Core/Injector/Factory.php

-    public function create($service, array $params = []);
+    public function create(string $service, array $params = []): ?object;


This isn't strictly necessary - but it does make things a bit clearer. Can remove this change if it's causing headaches.

GuySartorelli · 2024-07-04T02:40:57Z

src/Core/Cache/ApcuCacheFactory.php

+    private function isSupported(): bool
+    {
+        static $isSupported = null;
+        if (null === $isSupported) {
+            // Need to check for CLI because Symfony won't: https://github.com/symfony/symfony/pull/25080
+            $isSupported = Director::is_cli()
+                ? filter_var(ini_get('apc.enable_cli'), FILTER_VALIDATE_BOOL) && ApcuAdapter::isSupported()
+                : ApcuAdapter::isSupported();
+        }
+        return $isSupported;
    }


This is moved here from DefaultCacheFactory

GuySartorelli · 2024-07-04T02:41:37Z

src/Core/Cache/AbstractCacheFactory.php

Most of this class is moved here from DefaultCacheFactory

GuySartorelli · 2024-07-04T02:43:03Z

_config/cache.yml

@@ -7,7 +7,6 @@ SilverStripe\Core\Injector\Injector:
    constructor:
      args:
        directory: '`TEMP_PATH`'
-        version: null


The version is effectively a flush mechanism - if the version changes, all cache with the previous version is invalidated.

Given this config isn't available when instantiating manifest cache, I've moved this to being an environment variable. In practice I don't think anyone would ever bother setting it since Silverstripe CMS has so many ways to flush cache.

GuySartorelli · 2024-07-04T04:16:38Z

src/Core/Cache/CacheFactory.php

     */
-    public function create($service, array $params = []);
+    public function create(string $service, array $params = []): CacheInterface;


Note this is PSR16 - this is intended for direct use.

GuySartorelli · 2024-07-04T04:18:12Z

src/Core/Cache/InMemoryCacheFactory.php

+    /**
+     * Create a PSR6-compliant cache adapter
+     */
+    public function createPsr6(string $service, array $params = []): CacheItemPoolInterface;


PSR6 specifically required so it can be bundled in a ChainAdapter along-side the filesystem cache adapter.
This is in contrast to the create() method which must be PSR16 for direct use.

GuySartorelli · 2024-07-09T00:06:05Z

tests/php/Core/Cache/CacheTest.php

The YAML for these is no longer valid, and these tests would now fail if memcached isn't installed and apcu isn't enabled for CLI.

These are covered through the new DefaultCacheFactoryTest anyway.

GuySartorelli · 2024-07-09T00:51:07Z

tests/php/Core/Injector/InjectorTest.php

The changes here are a natural consequence of strict typing in the Factory interface. It's documented in the changelog.

GuySartorelli · 2024-07-09T03:34:22Z

.github/workflows/ci.yml

+      phpunit_skip_suites: framework-cache-only
+      # Ensure we test all in-memory cache factories provided in core
+      extra_jobs: |
+        - phpunit: true
+          phpunit_suite: framework-cache-only
+          install_inmemory_cache_exts: true


This works with the two CI PRs to ensure we install everything we need to be able to include all three in-memory cache adapters in the new DefaultCacheFactoryTest test.

emteknetnz · 2024-07-11T03:03:48Z

src/Core/Cache/RedisCacheFactory.php

+            throw new RuntimeException('Redis is not supported in the current environment. Cannot use Redis cache.');
+        }
+
+        $dsn = Environment::getEnv('SS_REDIS_DSN');


Would it be worth simply having SS_IN_MEMORY_CACHE_DSN and checking it starts with redis instead on splitting out SS_REDIS_DSN and SS_MEMCACHED_DSN, given that there's only a single SS_IN_MEMORY_CACHE_FACTORY?

I think it's cleaner if they're separate, given the factories are separate. Each factory looks for its own DSN. I wouldn't want the memcached factory to be looking for a DSN variable that's shared with Redis, that sounds messy to me.

.github/workflows/ci.yml

src/Core/Cache/DefaultCacheFactory.php

tests/php/Core/Cache/DefaultCacheFactoryTest.php

src/Core/Cache/DefaultCacheFactory.php

GuySartorelli marked this pull request as draft July 4, 2024 02:27

GuySartorelli mentioned this pull request Jul 4, 2024

Cache is not correctly shared between CLI and non-CLI #11145

Closed

10 tasks

GuySartorelli force-pushed the pulls/6/in-memory-cache branch from 82cf8c1 to 502fee6 Compare July 4, 2024 02:30

This was referenced Jul 4, 2024

FIX Respect strict-typing of Factory interface silverstripe/silverstripe-assets#620

Merged

FIX Respect strict-typing of Factory interface silverstripe/silverstripe-versioned#455

Merged

GuySartorelli commented Jul 4, 2024

View reviewed changes

src/Core/Cache/AbstractCacheFactory.php Outdated

Copy link

Member Author

GuySartorelli Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this class is moved here from DefaultCacheFactory

GuySartorelli commented Jul 4, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/6/in-memory-cache branch 2 times, most recently from c0e7664 to f8c11d8 Compare July 4, 2024 04:06

GuySartorelli commented Jul 4, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/6/in-memory-cache branch from f8c11d8 to 839cb92 Compare July 4, 2024 04:17

GuySartorelli commented Jul 4, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/6/in-memory-cache branch from 839cb92 to c75413f Compare July 9, 2024 00:04

GuySartorelli commented Jul 9, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/6/in-memory-cache branch from c75413f to feec1d2 Compare July 9, 2024 00:49

GuySartorelli commented Jul 9, 2024

View reviewed changes

FIX Ensure cache is shared between CLI and webserver

2321737

GuySartorelli force-pushed the pulls/6/in-memory-cache branch from feec1d2 to 2321737 Compare July 9, 2024 00:58

GuySartorelli marked this pull request as ready for review July 9, 2024 01:40

GuySartorelli commented Jul 9, 2024

View reviewed changes

emteknetnz requested changes Jul 11, 2024

View reviewed changes

MNT Test with all cache factories in CI

ed321f8

GuySartorelli force-pushed the pulls/6/in-memory-cache branch from ed04e03 to ed321f8 Compare July 11, 2024 21:30

emteknetnz approved these changes Jul 12, 2024

View reviewed changes

emteknetnz merged commit 386ff5a into silverstripe:6 Jul 12, 2024

emteknetnz deleted the pulls/6/in-memory-cache branch July 12, 2024 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Ensure cache is shared between CLI and webserver #11300

FIX Ensure cache is shared between CLI and webserver #11300

GuySartorelli commented Jul 4, 2024 •

edited

Loading

GuySartorelli Jul 4, 2024

GuySartorelli Jul 4, 2024

GuySartorelli Jul 4, 2024

GuySartorelli Jul 4, 2024

GuySartorelli Jul 4, 2024

GuySartorelli Jul 4, 2024

GuySartorelli Jul 9, 2024

GuySartorelli Jul 9, 2024

GuySartorelli Jul 9, 2024

emteknetnz Jul 11, 2024

GuySartorelli Jul 11, 2024 •

edited

Loading

		public function create($service, array $params = []);
		public function create(string $service, array $params = []): ?object;

FIX Ensure cache is shared between CLI and webserver #11300

FIX Ensure cache is shared between CLI and webserver #11300

Conversation

GuySartorelli commented Jul 4, 2024 • edited Loading

Description

Requires

Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

GuySartorelli commented Jul 4, 2024 •

edited

Loading

GuySartorelli Jul 11, 2024 •

edited

Loading