#41333 closed enhancement (fixed)
Implement `wp_initialize_site()` and `wp_uninitialize_site()`
Reported by: | flixos90 | Owned by: | flixos90 |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Networks and Sites | Keywords: | ms-roadmap, needs-patch |
Focuses: | multisite | Cc: |
Description
As summarized in https://core.trac.wordpress.org/ticket/40364#comment:14, we'd like to have solid functions for installing a site in a multisite network as well as uninstalling it.
These functions should only take care of the "site-level" part of things, like:
- creating/dropping database tables
- populating options
- setting up initial content
The idea is to only be required to call wp_insert_site()
and then wp_install_site()
to get a new site set up.
Before we start working on the two functions, we should discuss what exactly it needs to do. This will have to match what currently happens spread out between wpmu_create_blog()
and another function it calls, install_blog()
. We should aim to make this consistent and also think about ways to enhance these functions and the functions they use, for example by providing filters to adjust some of these defaults.
This will be a rather comprehensive task, so it does not necessarily need to go in at a similar time as the changes from #40364. We should probably even wait to start it until those changes have been completed. This ticket should just be on the horizon for now, I'd say.
Attachments (4)
Change History (45)
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
#5
@
7 years ago
- Keywords early added
- Owner set to flixos90
- Status changed from new to assigned
This should be a 5.0 priority, together with #40364.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
6 years ago
#12
@
6 years ago
- Keywords has-patch needs-unit-tests needs-testing dev-feedback added; 2nd-opinion removed
41333.diff is a first take on this:
- A
wp_install_site
action is introduced and fired after the site's DB row has been inserted and after any other related functionality has been executed (via thewp_insert_site
hook). - A
wp_uninstall_site
action is introduced and fired before the site's DB row will be deleted and before any other related functionality will be executed (via thewp_delete_site
hook). - A new
wp_install_site()
function is hooked intowp_install_site
. It receives the site object and any arguments passed towp_insert_site()
that are not part of the site's DB row fields. - A new
wp_uninstall_site()
function is hooked intowp_uninstall_site
. It receives the site object. - The temporary removal of the action
wp_update_blog_public_option_on_site_update()
from theupdate_blog_public
hook has been moved intowp_insert_site()
. This is necessary since, when thewp_insert_site
hook fires, the database tables won't exist yet. - The removal of site metadata (
blogmeta
) is now part ofwp_delete_site()
. This data is in network scope, so it should be covered by this function, whilewp_uninstall_site()
covers the site scope data. wp_install_site()
is arguably the most complex part of the patch. It contains logic that was previously part ofwpmu_create_blog()
. Moreover,install_blog()
has been deprecated, and its logic is now also part ofwp_install_site()
, in a more flexible way.populate_options()
now accepts an optional$options
array parameter that allows to override some of the initial options with custom values. This parameter is used inwp_install_site()
, which allows to get rid of some unnecessaryupdate_option()
calls later on.wp_uninstall_site()
contains logic that was previously part ofwpmu_delete_blog()
. It removes users from the site, drops the DB tables and removes the uploads directory. It explicitly does not support the$drop
parameter, as uninstalling a site is equal to dropping everything. It's thus not a direct replacement forwpmu_delete_blog()
because it makes more sense to deal with setting thedeleted
flag and actually deleting separately.wpmu_create_blog()
now simply callswp_insert_site()
, passing the$title
,$user_id
, and$meta
arguments to it as necessary.wpmu_delete_blog()
now simply callswp_delete_site()
if$drop
is true. Sincewpmu_delete_blog()
removes all users from the site even if it only sets thedeleted
flag, that code may be executed twice (wp_uninstall_site()
needs to remove users as well). This is not an issue though, since after the first run those users will already have been removed.- Any default option that relies on some network value now uses the network that is specified for the new site. It previously would look up the current network, which may not always be the right one (usually it would, but not always).
- The unrelated function
install_blog_defaults()
has been moved toms-deprecated.php
. It has been deprecated for a long time, but for some reason was still inms-functions.php
.
Future follow-up stuff (that should not be part of this ticket):
wpmu_create_blog()
andwpmu_delete_blog()
should no longer be used by core. However I think it's best to make these changes in a release after 5.0, because we wanna make sure the code we have is really backward-compatible first.- The two functions should then probably be deprecated too.
Open questions from my end:
- In
wp_install_site()
, passing an empty user ID currently results in aWP_Error
. I wonder whether we should use a default, for example the network administrator of the site's network. That would be a good fallback IMO, since that user has access anyway. - Is the order of the actions the right choice?
- For inserting a site: DB transaction ->
wp_insert_site
hook ->wp_install_site
hook - For deleting a site:
wp_uninstall_site
hook -> DB transaction ->wp_delete_site
hook
- For inserting a site: DB transaction ->
This whole change needs quite a bit of testing, so please apply the patch and play around with it. It's not that big actually, a lot has been copy-paste only. The most important thing we need to ensure is that site installation and uninstallation (DB tables, correct initial options and content, uploads directory) work correctly.
Unit tests will be provided after some initial feedback and discussion on whether this is on the right track.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
6 years ago
#14
@
6 years ago
- Summary changed from Implement `wp_install_site()` and `wp_uninstall_site()` to Implement `wp_initialize_site()` and `wp_uninitialize_site()`
Results from today's office hours (https://wordpress.slack.com/archives/C03BVB47S/p1533658006000406) and next changes to implement:
wp_install_site
should be renamed towp_initialize_site
.wp_uninstall_site
should be renamed towp_uninitialize_site
.- A
wp_is_site_initialized()
function should be introduced that looks up the site's DB tables, with a short-circuit hook. wpmu_new_blog
should be deprecated and moved intowp_insert_site()
.delete_blog
should be deprecated and moved intowp_delete_site()
.
#15
@
6 years ago
41333.2.diff makes the following changes (as described above):
- Use
wp_initialize_site
andwp_uninitialize_site
instead ofwp_install_site
andwp_uninstall_site
. - Update documentation to reflect the above wording too.
- Introduce
wp_is_site_initialized()
and apre_wp_is_site_initialized
hook inside it. Use that function inwp_update_blog_public_option_on_site_update()
so that that function can always be hooked in regardless of whether a site is initialized or not. - Deprecate
wpmu_new_blog
and move it intowp_insert_site()
. - Deprecate
delete_blog
anddeleted_blog
and move them intowp_delete_site()
. Since the hooks still need to be executed when the site is not deleted, but only itsdeleted
flag is set,wpmu_delete_blog()
flow has been adjusted and it conditionally runs those actions as well. The parts where$drop
is possibly changed due to restrictions has been moved to the top of the function, since otherwise it causes unexpected issues: It does that at the moment, when you callwpmu_delete_blog()
for the main site with$drop
set to true, thedelete_blog
filter will receive true although the value will actually be set to false afterwards. The change I made here fixes this. Other than that, the flow inwpmu_delete_blog()
remains completely the same as before.
I'd like to discuss whether we should introduce pre-filters for inserting, updating and deleting sites. Those would allow both short-circuiting, but also allow to perform actions before anything is changed. Furthermore, I deprecated delete_blog
referring to wp_delete_site
as replacement, however a pre_wp_delete_site
filter would be a more accurate one.
Another thing I'm asking myself is whether the wp_insert_site
hook or wp_initialize_site
hook should be run first.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
6 years ago
#20
@
6 years ago
This is looking really good, @flixos90 - thank you!
I'd like to discuss whether we should introduce pre-filters for inserting, updating and deleting sites. Those would allow both short-circuiting, but also allow to perform actions before anything is changed. Furthermore, I deprecated delete_blog referring to wp_delete_site as replacement, however a pre_wp_delete_site filter would be a more accurate one.
I changed my mind a bit on pre-filters during today's Slack conversation. I think as long as we can define the ways that people can be flexible with the site creation process, then we're good. It does sound like a short-circuit filter for site deletion is a good idea. Site insertion can already be overridden.
Another thing I'm asking myself is whether the wp_insert_site hook or wp_initialize_site hook should be run first.
We just chatted through this in Slack and it seems that we're all on the same page with wp_insert_site
being fired first.
And one additional note from a review of the patch:
- It seems like the addition of the
$options
parameter topopulate_options()
might deserve a ticket of its own.
#21
@
6 years ago
Next steps, based on feedback and multisite meeting discussion:
- open extra ticket for adding an optional
$options = array()
parameter topopulate_options()
- open extra ticket for adding an optional
$roles = array()
parameter topopulate_roles()
- open extra ticket for adding a
populate_site_meta( $meta = array() )
function inwp-admin/includes/schema.php
- open extra ticket for adding a
populate_network_meta( $meta = array() )
function inwp-admin/includes/schema.php
(code for that is currently part ofpopulate_network()
) - in
wp_initialize_site()
, merge options fromoptions
argument with default options and pass them topopulate_options()
- add support for extra roles on a new site to
wp_initialize_site()
, viaroles
argument - add support for site meta on a new site to
wp_initialize_site()
, viameta
argument - make site initialization arguments filterable, with a new
wp_initialize_site_args
filter - add
wp_validate_site_deletion
action, where folks can amend aWP_Error
in order to prevent site deletion under certain circumstances
#25
@
6 years ago
- Keywords needs-dev-note added; dev-feedback removed
41333.3.diff includes the following improvements:
- In
wp_initialize_site()
, passedoptions
are directly passed topopulate_options()
for efficient insertion. - In
wp_initialize_site()
, ameta
argument is now supported, as key => value pairs, which are then passed topopulate_site_meta()
. - Introduce a
wp_initialize_site_args
filter to adjust arguments passed to site initialization on the fly. - Introduce a
wp_validate_site_deletion
action, that receives aWP_Error
object and allows to cancel site deletion. - Fix a critical bug where sites with an empty user ID would not be initialized. This was previously possible and needs to remain that way.
- Adjust the two utility functions hooked into
wpmu_new_blog
(which is deprecated) to be hooked intowp_initialize_site
instead. A tiny bit of code has been adjusted in both functions to support the different types of parameters from the new action.
As of this patch, all existing unit tests pass without issues.
There are only a few things left to do:
- Complete #44894, and then add support for custom
roles
towp_initialize_site()
. - Add tests for
wp_initialize_site()
. - Add tests for
wp_uninitialize_site()
. - Add tests for
wp_is_site_initialized()
. - Add tests to ensure
wp_insert_site()
triggerswp_initialize_site()
with proper arguments correctly. - Add tests to ensure
wp_delete_site()
triggerswp_uninitialize_site()
correctly. - Remove all actions from
wp_initialize_site
for allwp_insert_site()
tests that don't interact with the site's scope at all, to significantly improve tests performance. - Add tests for the new
wp_validate_site_deletion
action.
#26
@
6 years ago
I reviewed the latest patch, here is my feedback.
if ( 'https' === parse_url( get_network_option( $network->id, 'siteurl' ), PHP_URL_SCHEME ) ) {
should become
if ( 'https' === parse_url( $network->siteurl, PHP_URL_SCHEME ) ) {
The function install_blog_defaults
needs this
_deprecated_function( __FUNCTION__, '5.0.0' );
Breaks this into multiple lines to make it more readable.
$blog_id = wp_insert_site(
array_merge(
$site_data,
array(
'title' => $title,
'user_id' => $user_id,
'options' => array_diff_key( $options, array_flip( $site_data_whitelist ) ),
)
)
);
becomes
$site_data_default = array(
'title' => $title,
'user_id' => $user_id,
'options' => array_diff_key( $options, array_flip( $site_data_whitelist ) ),
);
$blog_options = array_merge( $site_data, $site_data_default );
$blog_id = wp_insert_site( $blog_options);
I would change the following as well
$result = false;
$suppress = $wpdb->suppress_errors();
if ( $wpdb->get_results( "DESCRIBE {$wpdb->posts}" ) ) {
$result = true;
}
$wpdb->suppress_errors( $suppress );
becomes
$suppress = $wpdb->suppress_errors();
$result = $wpdb->get_results( "DESCRIBE {$wpdb->posts}" );
$wpdb->suppress_errors( $suppress );
#27
@
6 years ago
I would also have a look into change how the unit test factories, so you can pass an param that would disable the tables being created.
The new param create the table and is defaults to true.
$id = $factory->blog->create( $id, true );
where as this
$id = $factory->blog->create( $id, false );
would not create the extra table.
This should really speed up some of the unit tests running.
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
6 years ago
#30
@
6 years ago
Quick summary of yesterday's multisite chat, we decided to not pursue adding specific support for $roles
to wp_initialize_site()
for now (related: #44894). There are a few complex issues related to that, and we don't wanna expose a specific interface to interact with it that would make developers assume that everything easily works as expected.
As a result, the only remaining part here is solid test coverage for the new functions.
#31
@
6 years ago
- Keywords has-unit-tests added; needs-unit-tests needs-testing removed
41333.4.diff introduces comprehensive tests for the new functions. It also addresses the issues that @spacedmonkey outlined earlier.
#34
@
6 years ago
Minor, but should this be WP_Error::has_errors()
instead of ! empty( WP_Error::$errors )
. in ms-blogs line 598?
#35
@
6 years ago
@TimothyBlynJacobs Good catch! Same in line 881. Will fix this in a later commit, as it's not critical.
These tickets belong to our planned roadmap (a few of them not per final decision), so flagging with a keyword for better overview.