Make WordPress Core

Opened 13 years ago

Closed 5 days ago

Last modified 5 days ago

#23290 closed defect (bug) (fixed)

When using switch_to_blog() with a persistent object cache that lacks wp_cache_switch_to_blog() support, non-persistent groups are not maintained

Reported by: markjaquith's profile markjaquith Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 7.0 Priority: low
Severity: normal Version: 3.5
Component: Cache API Keywords: has-patch needs-testing 2nd-opinion
Focuses: multisite Cc:

Description

If you're using a persistent object cache that lacks the new wp_cache_switch_to_blog() support, WordPress core elses into a complete cache clobbering branch. It is smart about grabbing the global groups and re-adding those after wp_cache_init() is called, but it doesn't do the same for non-persistent groups. Those are a hardcoded array. So if you call switch_to_blog(), you would lose any custom-added non-persistent groups.

Attachments (6)

23290.patch (4.6 KB) - added by ethitter 13 years ago.
23290-2.patch (31.0 KB) - added by johnjamesjacoby 4 weeks ago.
23290-3.patch (5.3 KB) - added by johnjamesjacoby 2 weeks ago.
23290-4.patch (5.6 KB) - added by johnjamesjacoby 2 weeks ago.
23290-phpunit.patch (26.0 KB) - added by johnjamesjacoby 12 days ago.
Unit test file for patches 3 & 4
23290-5.patch (40.2 KB) - added by johnjamesjacoby 11 days ago.

Download all attachments as: .zip

Change History (31)

#1 @johnjamesjacoby
13 years ago

  • Cc johnjamesjacoby added

#2 @nacin
13 years ago

  • Version set to 3.0

Curiously, not a regression — existed back when this was just a wp_cache_init() call too.

#3 @ethitter
13 years ago

  • Cc erick@… added

#4 @stevenkword
13 years ago

  • Cc stevenword@… added

@ethitter
13 years ago

#5 @ethitter
13 years ago

  • Keywords has-patch needs-testing added

@stevenkword and I worked on 23290.patch during the Boston WP Hack Day.

We introduced a get_non_persistent_groups() method in Core's WP_Object_Cache to handle the core use, and to be implemented by authors of object cache drop ins. Initially we tried to include universal support for getting the non-persistent cache groups, but review of the APC Object Caching plugin revealed that the structure of $global_groups can vary between persistent backends.

The switching fallback that was present in switch_to_blog() and restore_current_blog() was abstracted into _ms_cache_switch_fallback() for consistency, and logic to use the persistent group getter closely mirrors the handling of global groups already in those functions.

#6 @nacin
13 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

I like this; let's address it in 3.7.

#7 @wonderboymusic
13 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#8 @nacin
12 years ago

  • Keywords 3.7-early removed
  • Milestone changed from 3.7 to 3.8

Let's make sure ryan looks this over.

#9 @wonderboymusic
12 years ago

  • Keywords needs-unit-tests added

#10 @nunomorgadinho
12 years ago

Any suggestions on what a unit test for this one should include? Maybe we could even separate it into different unit tests. I'm unsure and would appreciate feedback as I would be interested in working on it.

Last edited 12 years ago by nunomorgadinho (previous) (diff)

#11 @nunomorgadinho
12 years ago

  • Cc nuno.morgadinho@… added

#12 @wonderboymusic
12 years ago

  • Milestone changed from 3.8 to Future Release

No unit tests - no Ryan blessing

#13 @jeremyfelt
12 years ago

  • Focuses multisite added
  • Milestone changed from Future Release to 3.9

We probably have an opportunity to get some unit tests in for this in 3.9.

@nunomorgadinho, if you are still interested in working on this, please feel free. It's probably worth looking at the current code in phpunit/tests/cache.php for examples of testing switch_to_blog(). We would then want to use additional tests for the pieces that this patch touches. Don't hesitate to provide any first attempts or stop in #wordpress-dev to go over approach.

#14 @jeremyfelt
12 years ago

This is probably going to miss 3.9 unless we can get some testing and tests in here.

#15 @nacin
12 years ago

  • Milestone changed from 3.9 to Future Release
  • Priority changed from normal to low

There are a number of Cache API things we'd like to do, so moving this out of 3.9.

It's much, much better for object caches to introduce wp_cache_switch_to_blog(), so this is fairly low priority.

#16 @chriscct7
10 years ago

  • Keywords needs-refresh added

#17 @r1k0
5 weeks ago

  • Keywords needs-testing removed

Patch does not apply cleanly to current trunk. .rej files are generated for wp-includes/cache.php and wp-includes/ms-blogs.php. Patch needs a refresh to match trunk. Removed needs-testing.

Error:

Running "patch:https://core.trac.wordpress.org/attachment/ticket/23290/23290.patch" (patch) task
patching file wp-includes/cache.php
Hunk #1 FAILED at 608.
1 out of 1 hunk FAILED -- saving rejects to file wp-includes/cache.php.rej
patching file wp-includes/ms-blogs.php
Hunk #1 FAILED at 518.
Hunk #2 FAILED at 579.
Hunk #3 FAILED at 626.
3 out of 3 hunks FAILED -- saving rejects to file wp-includes/ms-blogs.php.rej

#18 @johnjamesjacoby
4 weeks ago

  • Keywords needs-testing added; needs-unit-tests needs-refresh removed
  • Milestone set to 7.0
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

23290-2.patch does the following:

  • Introduces a new wp_cache_switch_to_blog_fallback() function in ms-blogs.php that only gets called when wp_cache_switch_to_blog() does not exist
  • Adds multisite tests for wp_cache_switch_to_blog_fallback() for defaults, custom groups, edge cases, and some integration scenarios

This ticket was mentioned in Slack in #core-test by mohkatz. View the logs.


3 weeks ago

#20 @johnjamesjacoby
2 weeks ago

23290-3.patch is a shower-thought approach:

  • Add a fallback wp_cache_switch_to_blog() function in cache-compat.php
  • Move the fallback code out of switch_to_blog() and restore_current_blog() and stop using function_exists()
  • Allows us to catch another edge case where wp_cache_switch_to_blog() doesn't exist but $wp_object_cache->switch_to_blog() does

This works because cache.php is included before cache-compat.php, so WordPress can easily identify internally that wp_cache_switch_to_blog() is missing when it should not be and then include its own fallback for it.

I really like this approach, because it also ensures that wp_cache_switch_to_blog() is always callable by plugins.

The only compromise with this patch iteration is: I could not see how to retroactively introduce a new switch_to_blog feature inside of wp_cache_supports() - it's a catch-22 because existing drop-in plugins would not have needed to announce/register their supporting this, its default return value is false, and it's already confusing enough without added logic inside of the compat wp_cache_supports() function.

Last edited 2 weeks ago by johnjamesjacoby (previous) (diff)

#21 @johnjamesjacoby
2 weeks ago

23290-4.patch is a riskier side quest - same as 23290-3.patch but:

  • Removes the hard-coded calls to wp_cache_switch_to_blog() inside of switch_to_blog() and restore_current_blog()
  • Adds wp_cache_switch_to_blog to the switch_blog hook, inside of ms-default-filters.php

This works because wp_cache_switch_to_blog() will always exist now, which allows us to decouple the Cache API from the Blog Switching API, similar to how the Roles API and wp_switch_roles_and_user() is.

I consider this risky because it hides an important function call that has always been an obvious one. But it is neat so I thought I'd suggest it too.

#22 @ozgursar
2 weeks ago

I only tested one the patches (23290-3.patch) that @johnjamesjacoby submitted.

Patch Testing Report

Patch Tested: https://core.trac.wordpress.org/attachment/ticket/23290/23290-3.patch

Environment

  • WordPress: 7.0-alpha-61215-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 145.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.4
  • MU Plugins: None activated
  • Plugins:
    • Code Snippets 3.9.5
    • SQLite Object Cache 1.6.1
    • Test Reports 1.2.1

Steps taken

  1. Enable multisite by adding the following values to the wp-config.php and add a second site to the network.
define( 'WP_ALLOW_MULTISITE', true );
define( 'MULTISITE', true );
define( 'SUBDOMAIN_INSTALL', false );
define( 'DOMAIN_CURRENT_SITE', 'localhost:8889' );
define( 'PATH_CURRENT_SITE', '/' );
define( 'SITE_ID_CURRENT_SITE', 1 );
define( 'BLOG_ID_CURRENT_SITE', 1 );
  1. Install and activate SQLite Object Cache plugin
  2. Edit /wp-content/object-cache.php and comment out the function wp_cache_switch_to_blog() between the lines 3084-3088 which implements the switch_to_blog()
function wp_cache_switch_to_blog( $blog_id ) {
   global $wp_object_cache;

   $wp_object_cache->switch_to_blog( $blog_id );
}
  1. Add the test snippet below via Code Snippets plugin or via functions.php
  2. Visit any page of Dashboard to view the test results
  3. ✅ Patch is solving the problem

Expected result

After applying the patch, when switch_to_blog() is called on a multisite installation using a persistent object cache drop-in that does not implement wp_cache_switch_to_blog(), custom non-persistent cache groups should be preserved.

Specifically, a value stored in a custom non-persistent group (my_test_group) before the blog switch should still be retrievable after switch_to_blog() and restore_current_blog() are called. The admin notice should display a green success message confirming that the value test_value survived the switch, matching the value stored before the switch.

Additional Notes

Without the patch, the value is lost (returns false) because wp_cache_init() wipes the cache and non-persistent groups are not restored. With the patch, the fallback wp_cache_switch_to_blog() function added to cache-compat.php correctly saves and restores non-persistent groups, so the value survives.

Screenshots/Screencast with results

Before patch:
https://i.imgur.com/xgVlXqY.png

After patch:
https://i.imgur.com/kKyPtG9.png

Support Content

  • This test snippet is created with the help of Claude.ai
add_action( 'admin_notices', function () {
    if ( ! is_multisite() ) {
        echo '<div class="notice notice-warning"><p><strong>Trac #23290 Test:</strong> Multisite is required.</p></div>';
        return;
    }

    // Check a second site exists
    $sites = get_sites( [ 'number' => 2 ] );
    if ( count( $sites ) < 2 ) {
        echo '<div class="notice notice-warning"><p><strong>Trac #23290 Test:</strong> A second site is required. Please create one under My Sites → Network Admin → Sites → Add New.</p></div>';
        return;
    }

    $other_blog_id = (int) $sites[1]->blog_id;

    wp_cache_add_non_persistent_groups( [ 'my_test_group' ] );
    wp_cache_set( 'test_key', 'test_value', 'my_test_group' );

    $before = wp_cache_get( 'test_key', 'my_test_group' );

    switch_to_blog( $other_blog_id ); // switch to a DIFFERENT blog
    restore_current_blog();

    $after = wp_cache_get( 'test_key', 'my_test_group' );

    $survived = ( 'test_value' === $after );
    $type     = $survived ? 'notice-success' : 'notice-error';
    $status   = $survived
        ? '✅ PASS — Non-persistent group survived switch_to_blog() (patch working).'
        : '❌ FAIL — Non-persistent group was LOST after switch_to_blog() (bug confirmed).';

    echo '<div class="notice ' . $type . '"><p>'
        . '<strong>Trac #23290 — Non-Persistent Cache Groups Test</strong><br>'
        . $status . '<br>'
        . '<small>'
        . 'Switched to blog ID: <code>' . $other_blog_id . '</code><br>'
        . 'Value before switch: <code>' . var_export( $before, true ) . '</code><br>'
        . 'Value after switch: <code>' . var_export( $after, true ) . '</code>'
        . '</small>'
        . '</p></div>';
} );
Last edited 2 weeks ago by ozgursar (previous) (diff)

@johnjamesjacoby
12 days ago

Unit test file for patches 3 & 4

#23 @johnjamesjacoby
11 days ago

  • Keywords 2nd-opinion added
  • Version changed from 3.0 to 3.5

23290-5.patch is a combo of 3.patch, 4.patch, and phpunit.patch:

  • Adds wp_cache_switch_to_blog_fallback() to ms-blogs.php so it can be independently unit tested
  • Adds wp_cache_switch_to_blog() to cache-compat.php.php so it always exists
  • Adds Tests_Multisite_WpCacheSwitchToBlogFallback with around 25 new tests that the fallback is working
  • Moves wp_cache_switch_to_blog() to the switch_blog action

Critical eyes may notice that wp_cache_switch_to_blog_fallback() does not have a $blog_id parameter, even though one is passed into it inside of Tests_Multisite_WpCacheSwitchToBlogFallback::call_cache_switch_fallback(). This is intentional, because: the function doesn't currently need the parameter, if we add one later the tests are already covered, and PHP doesn't complain about it.

Tangentially, while manually stress testing this, I discovered that if switch_to_blog() gets called via sunrise.php bad things happen, because ms-default-filters.php isn't included until later, and wp_switch_roles_and_user() (and now wp_cache_switch_to_blog()) are hooked to the switch_blog action. Not certain if this is sanctioned or not, but my guess is not.


IMO, this is ready for 2nd opinion & commit. Maybe @jeremyfelt or @spacedmonkey?

Last edited 10 days ago by johnjamesjacoby (previous) (diff)

#24 @johnjamesjacoby
5 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 61760:

Cache API: Introduce fallback for when the wp_cache_switch_to_blog() function is missing.

This commit protects against an edge-case where a persistent object-cache drop-in plugin not including its own wp_cache_switch_to_blog() function would cause non-persistent cache groups to go missing when switching between sites, and includes the following changes:

  • A new wp_cache_switch_to_blog_fallback() function in ms-blogs.php which abstracts duplicated code from switch_to_blog() and restore_current_blog() for easier unit testing
  • A new wpCacheSwitchToBlogFallback.php file with approximately 25 new unit tests
  • Conditionally declares wp_cache_switch_to_blog() in cache-compat.php only if it does not already exist, either via core's cache.php or a drop-in plugin

With this change, WordPress no longer needs to check if the wp_cache_switch_to_blog() function exists (because it always will) so those checks have been removed.

Props ethitter, jeremyfelt, johnjamesjacoby, markjaquith, nacin, ozgursar, r1k0.

Fixes #23290.

#25 @johnjamesjacoby
5 days ago

In 61772:

Cache API: Handle numerically-keyed global_groups when switching sites.

This commit protects against another multisite cache edge-case where a persistent object-cache drop-in plugin (namely memcached) may use a numerically-keyed global_groups array instead of 'key' => true like the default object-cache class, and includes the following changes:

  • Use wp_is_numeric_array() inside of wp_cache_switch_to_blog_fallback() so that the global group names array are always properly formatted regardless of the caching back-end in use
  • Add private helper methods to Tests_Multisite_WpCacheSwitchToBlogFallback to properly format global group names, and tweak a few tests to make them more resilient to different caching back-ends

Follow up to r61760.

See #23290.

Note: See TracTickets for help on using tickets.