rhubarbgroup / redis-cache

A persistent object cache backend for WordPress powered by Redis. Supports Predis, PhpRedis, Relay, replication, sentinels, clustering and WP-CLI.
https://wordpress.org/plugins/redis-cache/
GNU General Public License v3.0
427 stars 149 forks source link

Prevent first time critical errors #458

Closed A5hleyRich closed 1 year ago

A5hleyRich commented 1 year ago

As discussed in Slack, we allow users to enable Redis Object Cache without determining if Redis is reachable. This can lead to 500 errors after the drop-in is placed in situ.

We should not allow Redis Object Cache to be enabled via the UI if Redis cannot be reached and surface any errors. Now might also be a good time to improve the Overview tab.

CleanShot 2023-04-24 at 13 24 56@2x CleanShot 2023-04-24 at 13 23 39@2x CleanShot 2023-04-24 at 13 30 25@2x CleanShot 2023-04-24 at 13 32 15@2x
A5hleyRich commented 1 year ago

We might want to do the same for the CLI command:

CleanShot 2023-04-24 at 14 07 58@2x
tillkruss commented 1 year ago

This is fantastic. Two tiny notes:

I think "Not enabled" instead of "Redis Object Cache not enabled" reads nicer.

If Redis is not reachable, and the user did not configure WP_REDIS_* constants then we should say, you need to RTFM with a link.

A5hleyRich commented 1 year ago

Implementation of this isn't as easy as I thought because WP_Object_Cache is defined in object-cache.php. For us to check the connection from the plugin, we need to be able to connect to Redis without object-cache.php being in place.

Here's what I'm thinking:

  1. Bring WP_Object_Cache into the plugin namespace
  2. Include WP_Object_Cache from object-cache.php

Do you see any issue with this @tillkruss?

tillkruss commented 1 year ago

Yeah, Object Cache Pro's architecture made this easy.

Do you see any issue with this @tillkruss?

This will break a lot of people's setups, because they consider object-cache.php a standalone thing.

Can we use the includes/object-cache.php somehow?

Can we just alter object-cache.php to make the connect_via_*() usable from the outside?

A5hleyRich commented 1 year ago

Can we use the includes/object-cache.php somehow?

This gets hairy pretty quickly because of the global function declarations and WP_Object_Cache as they're already defined in wp-includes/cache.php if an external object cache isn't in use. Just to be able to load includes/object-cache.php, we need to make sure we're not redeclaring anything, which in turn means we don't have access to the connection methods without extracting them from WP_Object_Cache. See:

https://github.com/rhubarbgroup/redis-cache/commit/ec8a4429770d6decfdee56e5ee84c6166ec0b65f

Any other thoughts? I'm leaning toward the safest approach, which will be to duplicate the connection logic in the plugin.

tillkruss commented 1 year ago

Yeah, ec8a442 is way too messy. Why don't we just use Predis and try to connect in a standalone function?

With the addition of our "db-errors.php" thing we will display a nicer error when something goes wrong when the user doesn't use Predis.