Closed stklcode closed 2 months ago
I like the direction where it goes; having Redis support is important.
I have two things that I believe are important to mention:
Version 6 allows to pass the parameters to the constructor. The filter could be placed much earlier maybe (before Line 215).
This one is just to keep in mind. The support for Redis is right now using PhpRedis exclusively. Predis might be a second good choice to implement right after.
I checked this branch, and it caches. 🎉 There are 2 small issues though:
The website runs on a WordPress multisite (in a multinetwork).
I believe there is no icon for the Redis in the symbols.svg (screenshot).
Correct. Applied a quick fix and duplicated the apc/memcached symbol again (memory bar or what ever it's supposed to represent).
The click on "Flush site cache" seems to have no effect.
Well, it does. However, the dashboard is not perfect here as it just outputs "used_memory"
But the most important: first page hit after flush receives an uncached page.
Maybe "used_memory_dataset" is more appropriate, but even this is ~23 KB right after startup and ~135 KB in the above scenario after flushing. Using shared Redis instances makes it even worse, these numbers are just server stats. (ref: https://redis.io/docs/latest/commands/info/)
Failed conditions
3.2% Duplication on New Code (required ≤ 3%)
Simply use "localhost" as default connection string instead of the URL-style "redis://localhost:6379" which is not accepted by PhpRedis.
With default setup I get an error like this:
(PHP 8.3.8, PhpRedis 5.3.7, Valkey 7.2.5 on 127.0.0.1:6379)
As PhpRedis requires separate arguments, we extend the hook and pass an array of up to 7 arguments, so we can override port, timeouts and other stuff as required.
Our new default is
[ 'localhost' ]
(implicitly port 6379), but the filter can return things like[ 'localhost', 16379 ]
or['127.0.0.1', 6379, 1, NULL, 0, 0, ['auth' => ['phpredis', 'phpredis']]]
and for convenience we also accept plain strings like'/path/to/redis.sock'
See https://github.com/phpredis/phpredis/tree/5.3.7?tab=readme-ov-file#connection
The sanitization routine is rather long. We might omit it and assume that admins configuring the filter know what they are doing... (though we all know that's somewhat optimistic)