Make WordPress Core

Opened 13 years ago

Last modified 2 days ago

#23290 assigned defect (bug)

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.0
Component: Cache API Keywords: has-patch needs-testing
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 (2)

23290.patch (4.6 KB) - added by ethitter 13 years ago.
23290-2.patch (31.0 KB) - added by johnjamesjacoby 7 days ago.

Download all attachments as: .zip

Change History (21)

#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
12 days 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
7 days 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.


2 days ago

Note: See TracTickets for help on using tickets.