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

[jeremyfelt]

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.

[scribu]

If we use update_site_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.

[jeremyfelt]

Good point.

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

[ryan]

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]

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

[ryan]

​[UT941]

[johnjamesjacoby]

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?

[ryan]

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.

[evansolomon]

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().

[ryan]

In [21378]:

Use Yoda conditions. Props evansolomon. see #21270

[nacin]

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.

[ryan]

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.

[ryan]

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

[ryan]

#21432 #21434