Fix redis failback on multisite

[jtsternberg]

In https://github.com/pantheon-systems/wp-redis/blob/master/object-cache.php#L1042-L1043:

// $wpdb->options can be unset before multisite loads
// It's safe to skip here if unset, because cache will be reinitialized when `$blog_id` is available

The problem with this logic is that, on multisite, the flushAll will never be called, even though it is needed (because the try block failed for any number of reasons). This pretty much defeats the purpose of the failback logic, if we're on multisite.

My suggestion is to instead, use $wpdb->sitemeta (and the correct columns for that table) if on multisite, so that the flushAll will still be called on multisite, if it is needed.

Possibly related: #72

[danielbachhuber]

My suggestion is to instead, use $wpdb->sitemeta (and the correct columns for that table) if on multisite, so that the flushAll will still be called on multisite, if it is needed.

I think this makes sense, but why did you refactor so much code?

[jtsternberg]

I tried to do it in logical commits to make it clear, but basically, it would have created a big mess to check for multisite and then change the query to match in every place it needed to happen. It makes sense to move them to their own separate methods, though you could argue that it could be done in one method if desired.

[jtsternberg]

Looks like travis failed, but tests appear to work fine for me locally: http://d.pr/1iDZ8

[danielbachhuber]

Looks like travis failed, but tests appear to work fine for me locally:

Look at the failing test. You need to run the tests locally against multisite in order to replicate the failure.

[danielbachhuber]

It makes sense to move them to their own separate methods, though you could argue that it could be done in one method if desired.

I think it makes more sense to do with a smaller changeset, because we don't gain much by refactoring. #75

[jtsternberg]

Agree to disagree. ;) Thanks for #75.

[joshkoenig]

To boil it down a bit, when running a site network, we can't rely on a query like:

SELECT option_value FROM {$wpdb->options} WHERE option_name='wp_redis_do_redis_failback_flush'

Because that will be routed to a site-specific location and the setting is network-wide?

Also, not sure I understand this statement:

The problem with this logic is that, on multisite, the flushAll will never be called, even though it is needed (because the try block failed for any number of reasons).

The try block is attempting to run a command against Redis. If it failed that's because the command failed. If calling flushAll fails in a way we don't catch, there are likely bigger problems. And neither of the patches address this flow at all. Am I missing something?

[jtsternberg]

Because that will be routed to a site-specific location and the setting is network-wide?

There is that, but the main issue is that $wpdb->options IS empty (on this line) when using multisite. It will never NOT be empty a that point in the load order. HOWEVER, $wpdb->options is NOT empty, and we can (and should for the reason you stated above) use that instead to populate this status/trigger.

The try block is attempting to run a command against Redis. If it failed that's because the command failed. If calling flushAll fails in a way we don't catch, there are likely bigger problems. And neither of the patches address this flow at all. Am I missing something?

I think I confused the issue. The point I was making there is the same point... Because $wpdb->options is empty on multisite, flushAll will never be called.