Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#21270 closed defect (bug) (fixed)

Use update_blog_option() when enabling/disabling themes per site in Multisite

Reported by: jeremyfelt's profile jeremyfelt Owned by: ryan's profile ryan
Milestone: 3.5 Priority: normal
Severity: minor Version:
Component: Cache API Keywords: has-patch
Focuses: Cc:

Description

Currently, when site/theme options are changed in site-themes.php, we use update_option( 'allowedthemes', $allowed_themes ).

This seems to cause an object cache related issue after the save and redirect when the table is displayed again. After clicking enable or disable next to a theme, no change is obvious, but the database is still updated.

When get_allowed_on_site() fires in class-wp-theme.php, and $current evaluates to false, we use get_blog_option( $blog_id, 'allowedthemes' ) to grab the data, which looks first to an object cache key that is not set on update in site-themes.php:

$key = $blog_id . '-' . $setting . '-blog_option'

When object cache is not available, this falls back to get_option(), which pulls the data as intended.

This patch modifies site-themes.php to use update_blog_option( $id, 'allowedthemes', $allowed_themes ). This continues to set the original update_option() but provides for the correct cache key/value as well.

Attachments (6)

21270.diff (481 bytes) - added by jeremyfelt 12 years ago.
21270.2.diff (745 bytes) - added by jeremyfelt 12 years ago.
21270.3.diff (7.1 KB) - added by ryan 12 years ago.
21270.4.diff (7.3 KB) - added by ryan 12 years ago.
21270-ut.diff (4.8 KB) - added by ryan 12 years ago.
21270.5.diff (1.1 KB) - added by evansolomon 12 years ago.

Download all attachments as: .zip

Change History (20)

@jeremyfelt
12 years ago

#1 @scribu
12 years ago

  • Component changed from Multisite to Cache
  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 3.5

If we use update_blog_option(), we can drop the switch_to_blog() / restore_current_blog() which would be done internally now.

It would make sense to do this even without the caching benefit, so +1.

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

@jeremyfelt
12 years ago

#2 @jeremyfelt
12 years ago

  • Keywords has-patch added; needs-patch removed

Good point.

21270.2.diff also removes switch_to_blog() and restore_current_blog() and replaces get_option() with get_blog_option().

#3 @ryan
12 years ago

wordpress.com uses switch_to_blog(), get_option(), restore_current_blog() in get_blog_option() and totally ignores the site-options cache. The site-options cache and get_blog_option() are insane.

@ryan
12 years ago

@ryan
12 years ago

@ryan
12 years ago

#4 @ryan
12 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [21357]:

Refactor *_blog_option() functions to use switch_to_blog(), restore_current_blog(), and the *_option() functions. Do not use site-options for blog option caching as this duplicated info and did not properly invalidate.

Props jeremyfelt
fixes #21270

#6 @johnjamesjacoby
12 years ago

Switching to blog for each individual option triggers my spidey-sense, that this may not perform very well. We rely on these functions heavily in BuddyPress to get root-blog options when on non-root sites. Do we have any benchmarks to indicate what difference this makes?

#7 @ryan
12 years ago

wordpress.com has done switch and restore for three years. It's a pretty good performance testbed. Regardless, I'll take a hit in performance over cache duplication and poisoning.

#8 @evansolomon
12 years ago

The logical comparisons for $id and get_current_blog_id() would make more sense to me in the opposite order (function then variable). That would be what Yoda suggests, but (more importantly I think) it would also make those lines easier to distinguish from the preceding lines that set $id = get_current_blog_id().

@evansolomon
12 years ago

#9 @evansolomon
12 years ago

  • Cc evan@… added

#10 @ryan
12 years ago

In [21378]:

Use Yoda conditions. Props evansolomon. see #21270

#11 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening this for some further discussion:

  • _blog_option() was never good, but these changes do highlight some problems because a switch_to_blog() clears the local cache. If you don't have a persistent backend, this means a single get_blog_option() wipes out most of your cache for the rest of the page.
  • We should remove all usage of _blog_option() in core.
  • We should deprecate _blog_option() in favor of "switch_to_blog(), get_option()" to ensure that people actually do realize that get_blog_option() is no less expensive.
  • We should improve our default caching backend to have a way to init it for a single blog, without losing the existing local cache.

#12 @ryan
12 years ago

nacin beat me to it. My comment:

Actually, this can cause quite a few extra queries when running without a persistent cache. switch_to_blog() calls wp_cache_init() which obliterates the wp cache instance. With a persistent cache this simply means some extra cache trips to repopulate the local cache after calling restore_current_blog(). Without persistence this means db queries to repopulate local cache. nacin and I discussed a softer init that maintains a stack of pointers to existing cache objects.

These functions should be deprecated and switch_to_blog()/get_option()/restore_to_blog() should be used instead. This way multiple option calls can be done within one switch_to_blog() instead of switching and restoring for every *_blog_option() call.

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

#13 @ryan
12 years ago

Two new tickets? One to deprecate *_blog_option() and eliminate core usage of them. One for a soft cache init.

#14 @ryan
12 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.