WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#25316 closed defect (bug) (fixed)

Inconsistent behavior in wpmu_current_site() when using persistent object cache

Reported by: gradyetc Owned by:
Milestone: 3.9 Priority: normal
Severity: minor Version: 3.0
Component: Networks and Sites Keywords: has-patch
Focuses: multisite Cc:

Description

We're running a multisite sub-directory install, using the Networks for WordPress to serve multiple domains from one install. We're also using memcached as a persistent object cache.

Since we need to support multiple networks we do not set the DOMAIN_CURRENT_SITE or PATH_CURRENT_SITE constants as instructed by the network install instructions.

We've noticed some odd behavior after a fresh install, before we add additional networks due to logic in wpmu_current_site().

Excerpting the relevant code from ms-load.php:

$current_site = wp_cache_get( 'current_site', 'site-options' );
if ( $current_site )
	return $current_site;

$sites = $wpdb->get_results( "SELECT * FROM $wpdb->site" ); // usually only one site
if ( 1 == count( $sites ) ) {
	$current_site = $sites[0];
	wp_load_core_site_options( $current_site->id );
	$path = $current_site->path;
	$current_site->blog_id = $wpdb->get_var( $wpdb->prepare( "SELECT blog_id FROM $wpdb->blogs WHERE domain = %s AND path = %s", $current_site->domain, $current_site->path ) );
	$current_site = get_current_site_name( $current_site );
	if ( substr( $current_site->domain, 0, 4 ) == 'www.' )
		$current_site->cookie_domain = substr( $current_site->domain, 4 );
	wp_cache_set( 'current_site', $current_site, 'site-options' );
	return $current_site;
}

When the 'current_site' cache item exists, the $domain and $path globals are not updated to reflect the cached values before returning. This is inconsistent with behavior exhibited when cached values do not exist. The end result is that trying to access pages on non-root sites (blogs) results in a 404 since the current site (blog) is not set correctly.

We get around this by installing another network and flushing the cache (after which the 'current_site' cache item is never set).

It's a minor issue, but I would think the $path and $domain variables should be set consistently.

Attachments (1)

25316.diff (522 bytes) - added by jeremyfelt 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @gradyetc
5 years ago

  • Summary changed from Inconsistent behavior in wpmu_current_site() that impacts external object caches to Inconsistent behavior in wpmu_current_site() when using persistent object cache

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Multisite

#3 @nacin
5 years ago

The end result is that trying to access pages on non-root sites (blogs) results in a 404 since the current site (blog) is not set correctly.

Could you explain this? I only see $domain and $path used in two places: ms_not_installed(), and during signups.

I kind of hate that it is intended for the $domain and $path variables to be global here. When fixing this bug, it would be nice to stop using these globals. For example, ms_not_installed() could receive $domain and $path as arguments instead.

#4 follow-up: @gradyetc
5 years ago

The end result is that trying to access pages on non-root sites (blogs) results in a 404 since the current site (blog) is not set correctly.

Could you explain this? I only see $domain and $path used in two places: ms_not_installed(), and during signups.

Lines 66 through 80 in wp-includes/ms-settings.php.

$blogname = htmlspecialchars( substr( $_SERVER[ 'REQUEST_URI' ], strlen( $path ) ) );
if ( false !== strpos( $blogname, '/' ) )
	$blogname = substr( $blogname, 0, strpos( $blogname, '/' ) );
if ( false !== strpos( $blogname, '?' ) )
	$blogname = substr( $blogname, 0, strpos( $blogname, '?' ) );
$reserved_blognames = array( 'page', 'comments', 'blog', 'wp-admin', 'wp-includes', 'wp-content', 'files', 'feed' );
if ( $blogname != '' && ! in_array( $blogname, $reserved_blognames ) && ! is_file( $blogname ) )
	$path .= $blogname . '/';
$current_blog = wp_cache_get( 'current_blog_' . $domain . $path, 'site-options' );
if ( ! $current_blog ) {
	$current_blog = get_blog_details( array( 'domain' => $domain, 'path' => $path ), false );
	if ( $current_blog )
		wp_cache_set( 'current_blog_' . $domain . $path, $current_blog, 'site-options' );
}
unset($reserved_blognames);

It derives $blogname from $path. If it isn't set appropriately by wpmu_current_site when called on line 48, it messes up everything that tries to populate $current_blog after it.

Took me a while to trace this ... globals are fun if you like surprises. :)

#5 in reply to: ↑ 4 @jeremyfelt
5 years ago

Replying to gradyetc:

It derives $blogname from $path. If it isn't set appropriately by wpmu_current_site when called on line 48, it messes up everything that tries to populate $current_blog after it.

I'm still trying to follow. $path and $domain are set before wpmu_current_site() based on $_SERVER['REQUEST_URI'] and $_SERVER['HTTP_HOST'] (starting on line 25). Are they not matching what should be in the $current_blog after wpmu_current_site() is finished?

#6 @gradyetc
5 years ago

Sorry for the lack of clarity -- perhaps an example would help:

http://example.com/ http://example.com/site/ http://example.com/site/about/

Where /site/ is a site in a sub-directory multisite install.

If you navigation to /site/about/, this is what happens:

Before the call to wpmu_current_site(), $path looks like this (on line 46): /site/

Inside wpmu_current_site() under normal conditions, $path will get reset to this: /

If this does not happen, the value for $path just before the attempts to set $current_blog starting on 74 is this: /site/about/

Instead of "/site/". (And also, $blogname is "about" instead of "site").

Since there is no blog with that path, it winds up using the root site instead. As a side note $domain doesn't actually change in wpmu_current_site(); so the real issue here is with $path.

Last edited 5 years ago by gradyetc (previous) (diff)

#7 @jeremyfelt
5 years ago

Ahh, got it now.

To clarify - this shows itself on a fresh sub-directory multisite install when DOMAIN_CURRENT_SITE and PATH_CURRENT_SITE are not defined as part of prep for a multi network install. As soon as more than one network exists, the cache is flushed and then never generated again.

@jeremyfelt
5 years ago

#8 follow-up: @jeremyfelt
5 years ago

25316.diff adds $path = $current_site->path; when the data is retrieved from cache, matching how $current_site is returned in other parts of wpmu_current_site().

#9 @SergeyBiryukov
5 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.7

#10 in reply to: ↑ 8 ; follow-up: @gradyetc
5 years ago

Replying to jeremyfelt:

25316.diff adds $path = $current_site->path; when the data is retrieved from cache, matching how $current_site is returned in other parts of wpmu_current_site().

This works. Another approach would be not relying on wpmu_current_site() to set that global at all. You could re-set $path in wp-includes/ms-settings.php with $current_site->path after it is set on line 48.

Looking at it more closely -- there are three lines in ms-settings.php that manipulate $path (44-46), right before the call to wpmu_current_site() clobbers it completely. This confused both of us...

Last edited 5 years ago by gradyetc (previous) (diff)

#11 in reply to: ↑ 10 @nacin
5 years ago

  • Milestone changed from 3.7 to Future Release

Replying to gradyetc:

Looking at it more closely -- there are three lines in ms-settings.php that manipulate $path (44-46), right before the call to wpmu_current_site() clobbers it completely. This confused both of us...

#12 @jeremyfelt
4 years ago

  • Component changed from Multisite to Networks and Sites
  • Focuses multisite added

This ticket was mentioned in IRC in #wordpress-dev by jeremyfelt. View the logs.


4 years ago

#15 follow-up: @jeremyfelt
4 years ago

I think this may be taken care of via #27003. $domain and $path are set once at the top of ms-settings.php.

@gradyetc Can you verify whether this issue is resolved in latest trunk?

#16 in reply to: ↑ 15 @gradyetc
4 years ago

Replying to jeremyfelt:

I think this may be taken care of via #27003. $domain and $path are set once at the top of ms-settings.php.

@gradyetc Can you verify whether this issue is resolved in latest trunk?

@jeremyfelt Confirmed -- this is no longer an issue in trunk. (Though I did find a related bug while confirming: https://core.trac.wordpress.org/ticket/27003#comment:38).

#17 @jeremyfelt
4 years ago

  • Milestone changed from Future Release to 3.9
  • Resolution set to fixed
  • Status changed from new to closed

Great, thanks @gradyetc. Good catch in the other ticket! We'll have to get that in soon.

Fixed in [27359]

Note: See TracTickets for help on using tickets.