WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 days ago

#40364 assigned enhancement

Improve site creation in multisite

Reported by: jeremyfelt Owned by: flixos90
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: ms-roadmap has-patch has-unit-tests
Focuses: multisite Cc:

Description

wpmu_create_blog() is responsible for a lot. It uses insert_blog(), install_blog(), wp_install_defaults() and many other things to accomplish the task of creating a site.

We should go through this process and determine what kind of improvements can be made as part of wpmu_create_blog() or in a new wp_create_site()/wp_insert_site() function that is more in tune with the objectives of site creation through the REST API. See #40365.

Attachments (6)

40364.diff (12.3 KB) - added by flixos90 6 months ago.
40364.2.diff (24.3 KB) - added by flixos90 3 months ago.
40364.3.diff (24.7 KB) - added by jeremyfelt 3 months ago.
40364.4.diff (24.7 KB) - added by flixos90 6 weeks ago.
40364.5.diff (24.7 KB) - added by flixos90 3 weeks ago.
40364.6.diff (32.0 KB) - added by flixos90 3 weeks ago.

Download all attachments as: .zip

Change History (38)

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

@flixos90
6 months ago

#6 @flixos90
6 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

40364.diff introduces new functions wp_insert_site(), wp_update_site() and wp_delete_site() (in ms-blogs.php). As briefly discussed in today's office-hours, the functions follow a more modern approach parameter-wise, as all of them accept a WP_Site object and return a WP_Site object as well (or WP_Error on failure):

  • wp_insert_site( $site ) accepts a WP_Site which must not have the $id property set. After having inserted the new site, it sets the $id property and returns the modified object (or WP_Error on failure).
  • wp_update_site( $site ) accepts a WP_Site which must have the $id property set. After having updated the site, it returns the object (or WP_Error on failure).
  • wp_delete_site( $site ) accepts a WP_Site which must have the $id property set. After having deleted the site, it sets the $id property to 0 and returns the modified object (or WP_Error on failure).

Passing objects around has several advantages:

  • The properties are already defined, so we don't need as much of complex default logic that we'd have otherwise.
  • It's straightforward. Instantiate a new site object, set properties and pass it to wp_insert_site().
  • You can work with the same object instance, less unnecessary object instantiations from redundant get_site() calls.
  • Passing objects around also opens up possibilities for more flexible filters as the objects contain much more information than just an ID. A good example is that we can now immediately pass the object to clean_blog_cache() without some weird logic such as the one in refresh_blog_details().

In addition to the above three functions, a new function is introduced for running the status transition hooks (previously located in update_blog_details()). This functionality is now part of wp_update_site(), but it should be outsourced to limit complexity of that function.

Furthermore the patch uses the three new functions: insert_blog() now wraps wp_insert_site(), update_blog_details() now wraps wp_update_site() and wpmu_delete_blog() calls wp_delete_site() internally.

This patch of course needs thorough review and testing (I haven't tested it at all tbh, will do that soon). Things to consider in particular:

  • Is the approach clear enough / flexible / stable enough?
  • Since objects are passed by reference, the input objects are directly modified. Is that acceptable, or should we clone them in all of the three functions to require the caller to use the returned object?
  • There are some //TODO annotations in the patch with further things to think about.

This ticket was mentioned in Slack in #core-restapi by flixos90. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


4 months ago

#12 @flixos90
4 months ago

See #38828 for things we should focus on improving with our new wp_update_site() function compared to the current way update_blog_details() works.

#13 @boonebgorges
4 months ago

#38828 was marked as a duplicate.

#14 @flixos90
3 months ago

  • Keywords needs-patch added; dev-feedback has-patch needs-testing removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to flixos90
  • Status changed from new to assigned

Relevant summary from last week's multisite meeting:

  • The patch needs to be changed to use the bad old $args pattern for now. A new pattern like passing around WP_Site objects should be discussed in a broader scope, affecting all similar core functions. It's not suitable here as it would just introduce a new pattern others might disagree with and thus cause inconsistency. I'll update the patch soon.
  • wp_insert_site(), wp_update_site() and wp_delete_site() deal with the wp_blogs table row only (a site in the network context). This ticket is only about implementing these three functions.
  • We'll also implement wp_install_site() and wp_uninstall_site(). The first will be responsible for setting up a site's DB tables, populating default options, initial content etc. The latter will be its counterpart, responsible for dropping everything from a site. This will be part of a separate ticket.
  • The last step will be to implement wp_create_site() (wraps wp_insert_site() and wp_install_site()) and wp_remove_site() (wraps wp_uninstall_site() and wp_delete_site()). The name for the latter could probably be more clear, so let's think about a more suitable name. Having those two functions available makes it easy for developers, as they then only need to call one function instead of two when creating/removing a site.

#15 @flixos90
3 months ago

  • Keywords ms-roadmap added

These tickets belong to our planned roadmap (a few of them not per final decision), so flagging with a keyword for better overview.

@flixos90
3 months ago

#16 @flixos90
3 months ago

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed

40364.2.diff is a patch for the new functions following the details that we recently discussed:

  • It introduces wp_insert_site( $data ), wp_update_site( $site_id, $data ) and wp_delete_site( $site_id ). All of these have the sole purpose of interacting with the wp_blogs database table.
  • Each of the three functions introduces its individual action hook where the site object (or in the case of wp_update_site() the current and previous site object) is passed to the hook.
  • Everything that goes beyond handling the database row and the related cache is not part of the respective function, but instead hooked in via the aforementioned action hooks. This includes the updating network counts process (via a new wp_maybe_update_network_site_counts_on_update() function) and the site status transition triggers (via a new wp_maybe_transition_site_statuses_on_update() function).
  • The new wp_maybe_transition_site_statuses_on_update() fires the hooks that were previously part of update_blog_details(). Two new make_public_blog and make_private_blog hooks have been introduced to complete the rest of them.
  • It has always been the case that when the public status changes, the respective site's blog_public option is changed accordingly. Since this is an action that happens in site scope and not in network scope (like all the rest), it must not be tied in with the functionality. Therefore the new make_public_blog and make_private_blog actions are used to hook in the new functions wp_make_blog_public_on_site_update() and wp_make_blog_private_on_site_update in order to take care of that functionality. In cases where we only wanna test interaction with wp_blogs without actually creating the site's individual DB tables (in unit tests for example), those hooks can be temporarily removed to not cause any issues.
  • insert_blog() now wraps wp_insert_site() and has been deprecated. wpmu_create_blog() has been adjusted to use wp_insert_site().
  • update_blog_details() now wraps wp_update_site().
  • wpmu_delete_blog() now uses wp_delete_site() to delete the wp_blogs row.
  • I ran all existing related unit tests to verify the changes work in a backward-compatible fashion. In addition I introduced several tests for the three new main functions.

Open questions (particularly important for the review):

  • Should wp_insert_site() and wp_update_site() contain any additional restrictions? At the moment, they do not check for example whether a domain and path combination doesn't already exist. I personally think that's fine as those are low-level functions and the wp_create_site() that we're going to introduce later will take care of these checks (that are currently handled in wpmu_create_blog() too and were not present in insert_blog()). The only check that is made in these functions is that a domain is always present. Furthermore if the path is empty, a simple slash is used.
  • When no site_id (network ID) is passed, the main network ID is used. Is that desired or should it rather use the current network ID?
  • update_blog_details() has now just become a simple wrapper for wp_update_site(). I generally would say that it should be deprecated, but since it is so widely used, I'm not sure that's a good step. What are the thoughts here? This is different to insert_blog() which is very rarely used.

#17 @flixos90
3 months ago

  • Milestone changed from Future Release to 4.9

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 months ago

@jeremyfelt
3 months ago

#19 follow-up: @jeremyfelt
3 months ago

This is looking good, thanks @flixos90! I added a slightly tweaked patch. Some comments/thoughts:

Should wp_insert_site() and wp_update_site() contain any additional restrictions?

  • It's tempting, but it's never happened at this low level before (insert_blog(), update_blog_details()), and I think we can avoid it. It's probably worth thinking about some more before we implement. Domain and path validation would specifically be interesting if it fit.
  • I did update the patch to trim the domain before checking emptiness.

When no site_id (network ID) is passed, the main network ID is used. Is that desired or should it rather use the current network ID?

  • I think we should use the current network ID if one is not specified. I would probably specify it every time myself, but I can imagine being confused in a multi network environment if the site ended up on another network when I didn't.
  • Related: In wp_update_site(), what case would lead to site_id being empty? It should already have one from the old site object. Are we protecting against false or 0 being passed to wp_update_site()? If so, maybe it should fall back to the old site's value instead of the main/current network?

update_blog_details() has now just become a simple wrapper for wp_update_site(). I generally would say that it should be deprecated, but since it is so widely used, I'm not sure that's a good step. What are the thoughts here?

  • How widely used is it outside of core? My gut says that we can probably deprecate it, but I'm not completely sure.

And the remaining:

  • This isn't a new problem, and could be another ticket, but should we think about adding a check for the new site's options table in WP_Site's get_details()? If I do wp_insert_site( $args ) and then immediately look for get_site( 2 )->home, I'll get an error because wp_2_options doesn't exist. Should I know enough as a dev to avoid that myself without a kinder warning?
  • Updated patch to remove a now unused $wpdb in update_blog_details()
  • The action names wp_insert_site, wp_update_site, and wp_delete_site don't necessarily clarify on their own (to me) that the action is fired after the function has performed its main task. What about wp_site_inserted, wp_site_updated, and wp_site_deleted? Or something else?

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 months ago

#22 in reply to: ↑ 19 ; follow-up: @flixos90
6 weeks ago

Replying to jeremyfelt:

Related: In wp_update_site(), what case would lead to site_id being empty? It should already have one from the old site object. Are we protecting against false or 0 being passed to wp_update_site()? If so, maybe it should fall back to the old site's value instead of the main/current network?

In 40364.4.diff the value falls back to the one previously set. This is indeed only useful when someone passes a falsy network ID value to wp_update_site().

My gut says that we can probably deprecate it, but I'm not completely sure.

Regarding deprecation, I'd like to add to this ticket what we discussed a few weeks ago: Let's only deprecate insert_blog() for now. The *_blog_details() functions can be considered at a later point, when this ticket (for update_blog_details(), #40180 and #40228 (for get_blog_details()) and #40201 (for refresh_blog_details()) have all been merged.

This isn't a new problem, and could be another ticket, but should we think about adding a check for the new site's options table in WP_Site's get_details()? If I do wp_insert_site( $args ) and then immediately look for get_site( 2 )->home, I'll get an error because wp_2_options doesn't exist. Should I know enough as a dev to avoid that myself without a kinder warning?

I think this all ends up being dev-education. As discussed, the functions part of this ticket are very low-level. The actual function to create a new site will be wp_create_site(), which will wrap wp_insert_site() and a future wp_install_site() and will be the de-facto replacement for wpmu_create_blog().

The action names wp_insert_site, wp_update_site, and wp_delete_site don't necessarily clarify on their own (to me) that the action is fired after the function has performed its main task. What about wp_site_inserted, wp_site_updated, and wp_site_deleted? Or something else?

I was following the examples in wp_insert_post() or wp_insert_comment() which use present tense, but I'm not opposed to another name. I don't like having only the past tense versions there - if they're the only filter, I'd rather use the function name. For other examples of how it's been done before, we might wanna look at wp_insert_term(): That one has to hooks, if we were going with that way, it would be insert_site and inserted_site - not sure how much sense that makes. I think I prefer the function name for the filter name, alongside with the docblock, which states that it is run after the process.

@flixos90
6 weeks ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 weeks ago

#24 in reply to: ↑ 22 @jeremyfelt
6 weeks ago

Replying to flixos90:

The action names wp_insert_site, wp_update_site, and wp_delete_site don't necessarily clarify on their own (to me) that the action is fired after the function has performed its main task. What about wp_site_inserted, wp_site_updated, and wp_site_deleted? Or something else?

I was following the examples in wp_insert_post() or wp_insert_comment() which use present tense, but I'm not opposed to another name. I don't like having only the past tense versions there - if they're the only filter, I'd rather use the function name.

Per today's Slack conversation - let's stick with simple actions that match the function names. wp_insert_site, wp_update_site, wp_delete_site (like @flixos90 already had). :)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


5 weeks ago

#28 @flixos90
5 weeks ago

  • Keywords early added
  • Milestone changed from 4.9 to Future Release

This is almost ready, but needs to wait for #41333 to be merged. Let's make it a 5.0 priority.

@flixos90
3 weeks ago

#29 @flixos90
3 weeks ago

40364.6.diff applies cleanly against the latest changes in trunk and makes the following adjustments:

  • Use wp_update_site() internally in update_blog_status().
  • Use the existing update_blog_public action hook in wp_maybe_transition_site_statuses_on_update().
  • Update version numbers to use 5.0.0.

Something we need to carefully examine is how the hooks when changing one of the site status fields are fired. In trunk, the hooks fire only if the value has changed when calling update_blog_details(), but they fire always when calling update_blog_status(), which I think doesn't make sense. Furthermore the update_blog_public hook only fires in update_blog_status() at all.
I think we should make this more consistent, which the above patch is a first take at. The hooks are now fired as part of wp_maybe_transition_site_statuses_on_update(), and they are only fired if their value has changed. While this may theoretically be a breaking change for very unusual setups (I don't know who would wanna do something when a status field is updated, but not actually changed), I think we should be good here - but it's something to discuss further of course. Note that for example I needed to adjust a few tests for these, since those would expect the hooks to run in update_blog_status() even when the value wasn't changed.

Note that in the current patch there are two tests that fail to pass, however I haven't been able to figure out the reason yet.

@flixos90
3 weeks ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


13 days ago

#31 @flixos90
13 days ago

  • Keywords early removed
  • Milestone changed from Future Release to 5.0

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 days ago

Note: See TracTickets for help on using tickets.