Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #118: On multisite-convert, don't assume site_id is 1 #122

Merged
merged 5 commits into from Oct 8, 2019

Conversation

@azito122
Copy link
Contributor

azito122 commented Apr 25, 2019

In the populate_network function, which is used by core multisite-convert, I observe the following code (line 1007 of wp-admin/includes/schema.php at v4.9.9):

if ( 1 == $network_id ) {
	$wpdb->insert( $wpdb->site, array( 'domain' => $domain, 'path' => $path ) );
	$network_id = $wpdb->insert_id;
} else {
	$wpdb->insert( $wpdb->site, array( 'domain' => $domain, 'path' => $path, 'id' => $network_id ) );
}

My understanding of the code in Core_Command.php is that multisite-convert is invoking populate_network(), and passing 1 as the site id ($network_id). As you can see, the WP core code then sets the network_id equal to the result of the INSERT query against wp_site. In most cases, this would be 1; however, if one has configured auto_increment_increment -- as is standard practice for database clusters -- then the wp_site table will store the site as id 4, and the code will generate the rest of the data using that same id.

From the perspective of core-command, this is mostly not a concern. However, it does appear that the constant SITE_ID_CURRENT_SITE is hardcoded with value 1. This produces an inconsistency between the config file and the database which is less than ideal. The solution is simple -- store the actual site_id/network_id and use that instead of a hardcoded 1. The only downside is that it means one extra database query (functions like get_main_network_id() appear to be accessing some kind of cache, or perhaps are falling back to default, and only return 1).

@azito122 azito122 requested a review from wp-cli/committers as a code owner Apr 25, 2019
src/Core_Command.php Outdated Show resolved Hide resolved
src/Core_Command.php Outdated Show resolved Hide resolved
@azito122 azito122 force-pushed the azito122:118-site-id branch 3 times, most recently from 02b8955 to 265f51b May 2, 2019
@azito122

This comment has been minimized.

Copy link
Contributor Author

azito122 commented May 2, 2019

I made two changes to the code:

  1. Use a direct DB call to get the site id. This should have been in my original pull request, but I missed committing the final changes I had made locally. I found that none of the multisite functions succeeded in obtaining the actual site id. Incidentally, this also fixes your first change request.

  2. Escaped the string as you suggested.

src/Core_Command.php Outdated Show resolved Hide resolved
src/Core_Command.php Outdated Show resolved Hide resolved
@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Aug 26, 2019

@azito122 Are you still interested in working on this?

azito122 and others added 3 commits Apr 24, 2019
Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
@azito122 azito122 force-pushed the azito122:118-site-id branch from 938864b to 156956c Aug 27, 2019
@azito122

This comment has been minimized.

Copy link
Contributor Author

azito122 commented Aug 27, 2019

@schlessera Yes. Apologies for going radio silent. I've made the last changes you requested and rebased.

schlessera added 2 commits Oct 8, 2019
@schlessera schlessera added this to the 2.0.7 milestone Oct 8, 2019
@schlessera schlessera merged commit 6532da4 into wp-cli:master Oct 8, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Oct 8, 2019

Thanks for the PR, @azito122 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.