WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 weeks ago

#29684 reviewing enhancement

Add get_main_site_id() function

Reported by: rmccue Owned by: jeremyfelt
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.9
Component: Networks and Sites Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

At the moment, the code that works out the main site for a network is locked away in ms-settings.php. We should move this out into a function that ms-settings.php can then use (ala get_site_by_path logic).

Attachments (5)

29684.diff (2.4 KB) - added by jeremyfelt 3 years ago.
29684.2.diff (6.2 KB) - added by flixos90 6 weeks ago.
29684.3.diff (2.0 KB) - added by spacedmonkey 4 weeks ago.
29684.4.diff (2.7 KB) - added by spacedmonkey 4 weeks ago.
29684.5.diff (7.9 KB) - added by flixos90 4 weeks ago.

Download all attachments as: .zip

Change History (27)

@jeremyfelt
3 years ago

#1 @jeremyfelt
3 years ago

  • Keywords dev-feedback has-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

I took a stab at this in 29684.diff.

I'm on the fence as to whether get_main_site() should return only the ID or a full site object. An ID fulfills the original need to set the network's main site ID. This could also be get_main_site_id().

The current bootstrap tests still pass, though we would need something more comprehensive for this function.

#2 @chriscct7
21 months ago

  • Keywords needs-unit-tests added

#3 @jeremyfelt
21 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 4.5

Depending on whether this or #34941 gets legs first, a refresh will be needed.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


18 months ago

#5 @jeremyfelt
18 months ago

  • Owner set to jeremyfelt
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by jorbin. View the logs.


18 months ago

#7 @jorbin
18 months ago

  • Milestone changed from 4.5 to Future Release

#8 @flixos90
2 months ago

I'd like to revive discussion around this.

I think since we have get_main_network_id(), we should also introduce get_main_site_id() (referring to @jeremyfelt's above proposal of only returning the site's ID instead of entire object). This function could as well be added to the functions.php file and return 1 if not a multisite.

Once the function exists, we can use it in ms_load_current_site_and_network() for populating the $blog_id property of the current network. We should then also use the function in the is_main_site() function and could then easily add support for a network ID parameter there, i.e. is_main_site( $site_id = null, $network_id = null ).

@flixos90
6 weeks ago

#9 @flixos90
6 weeks ago

  • Keywords has-unit-tests dev-feedback added; needs-unit-tests removed
  • Milestone changed from Future Release to 4.9

29684.2.diff is a new take on the get_main_site_id() function.

  • It introduces get_main_site_id( $network_id = null ).
    • If not multisite, it returns 1.
    • If invalid network parameter passed, it returns 0.
    • The actual lookup logic is the same that was previously located at the bottom of ms_load_current_site_and_network() with the exception that it now uses WP_Site_Query for improved performance. In case this lookup fails (which can only happen in case of a "broken" network without a matching site), the function returns 0.
  • It introduces a filter inside that function called get_main_site_id that receives the detected $main_site_id and the $network_id that it was detected for.
  • is_main_site() has been adjusted to use get_main_site_id() instead of only looking at the current network. In addition, it now supports an optional $network_id parameter as well that is simply forwarded to get_main_site_id().
  • The relevant code bit from ms_load_current_site_and_network() that was moved to the new function has been replaced by a call to that function.
  • Several unit tests have been added to ensure the new function works correctly. An additional unit test for is_main_site() with the $network_id parameter provided has been added too.

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.


4 weeks ago

#12 @flixos90
4 weeks ago

  • Owner changed from jeremyfelt to flixos90
  • Status changed from reviewing to accepted

We had a good discussion on this [today](https://wordpress.slack.com/archives/C03BVB47S/p1500916035737168).

I'll update the patch according to those ideas in a couple days.

#13 @spacedmonkey
4 weeks ago

My recommend is the following.

That we change WP_Network so that it uses a magic getter on blog_id, to dynamically load the blog id. Code found in the bootstrap process can be moved into the WP_Network class. Specially these lines


if ( ! $current_site->blog_id = wp_cache_get( 'network:' . $current_site->id . ':main_site', 'site-options' ) ) {
                        $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 ) );
                        wp_cache_add( 'network:' . $current_site->id . ':main_site', $current_site->blog_id, 'site-options' );

Instead of caching the result in object cache, it should be cached as a network option. The network option of main_site should be populared on network creation as well.

This add a massive benefit of being about to do this as well, get_network(33)->blog_id and be able to trust that blog_id is always set.

@spacedmonkey
4 weeks ago

@spacedmonkey
4 weeks ago

#14 @spacedmonkey
4 weeks ago

As a talking point, I have added 29684.4.diff. It doesn't add a get_main_site function. But does add the magic getter.

#15 follow-up: @flixos90
4 weeks ago

I agree that there should definitely be a magic getter for the main site ID of WP_Network. But the actual code should be part of get_main_site() so that it can be called independently. The magic getter could then call get_main_site().

#16 in reply to: ↑ 15 @spacedmonkey
4 weeks ago

That doesn't make any sense. Adding to WP_Network means it can be called from any context. The magic getter should do the fallback lookup. I think where possible we should be using BLOG_ID_CURRENT_SITE and BLOGID_CURRENT_SITE where possible, as these do not require a lookup.

Replying to flixos90:

I agree that there should definitely be a magic getter for the main site ID of WP_Network. But the actual code should be part of get_main_site() so that it can be called independently. The magic getter could then call get_main_site().

@flixos90
4 weeks ago

#17 @flixos90
4 weeks ago

  • Keywords dev-feedback removed
  • Status changed from accepted to assigned
  • Summary changed from Add get_main_site() function to Add get_main_site_id() function

29684.5.diff is a combination of all previous patches and follows mostly what we discussed on Monday:

  • It introduces a method get_main_site_id() (consistent with get_main_network_id()) that accepts a $network_id parameter.
    • The initial check is based on the multisite constants set, they basically overrule everything else. The network passed to the function must either have the same domain/path-combination or the same ID so that it is identified as that very network. Maybe we should make this even stricter and require it to have the same ID/domain/path-combination, let's think about this.
    • If the constants did not find the main site ID, we look at the current site and whether it has the same domain/path combination as the network. If so, we return that site's ID. This is similar behavior to what currently happens at the bottom of ms_load_current_site_and_network().
    • If that wasn't successful either, we do a cache lookup and otherwise use get_sites() do detect the proper main site. This as well is similar to what currently happens at the bottom of ms_load_current_site_and_network().
    • A filter get_main_site_id is run at the end so that the value can be adjusted. Something to think about here: The constant results currently do not go through this filter. Should they?
  • The existing is_main_site() method now accepts an additional parameter for the network ID and simply uses the new get_main_site_id() to get its result.
  • The bottom of ms_load_current_site_and_network() is now simplied since it simply needs to call get_main_site_id(). This also makes it simpler for authors of custom sunrise.php files.
  • WP_Network now includes a private get_main_site_id() method that is called by the magic getters for blog_id/site_id (as a reminder, these affect the same value). The method looks at the actual property and if it is not set calls get_main_site_id(). This enables us to access a valid $network->blog_id property from anywhere, even for a network that is not the "current" network.
  • Unit tests for get_main_site_id() and the related is_main_site() changes have been introduced as well.

There is one thing I did not do in this patch: I didn't introduce a main_site network option. Having had a second look, I don't think this makes sense. A simple multisite network doesn't need it anyway, and even for a multi-network it would be somewhat unreliable since it would only be set when calling get_main_site_id(). That is because it is impossible to set it in populate_network() when creating a network since in that function the ID of the site that belongs to the network is unknown unless multisite is initially set up. More importantly though is the following: Our original reason for introducing this was that it would be filterable. This is not a valid reason anymore though, since get_main_site_id() has a filter anyway which can be used for that purpose.

#18 @flixos90
4 weeks ago

  • Owner changed from flixos90 to jeremyfelt
  • Status changed from assigned to reviewing

#19 follow-up: @spacedmonkey
4 weeks ago

Saving in in network options, there were a couple of reason.

  1. To cache the result. The main site will likely never change. Because of this fact, it should be stored. This saves doing any queries on the blogs table. Take for example a network with 25k sites and 57 networks and no object cache enabled. That get_sites lookup could be expensive. Calling from a network option is a much faster lookup.
  1. Saving it in network option makes it easier to change the main site. You can change it using the filter, but then you have to maintain the filter function forever more. Seems clearer to just store it.
  1. Storing it in the network option makes the lookup much easier. How easy is it to find list of a main sites?
  1. In #37181 wp_load_core_site_options primes all the network options. So this main option would be primed in memory anyway.

I am not sure I understand your point about populate_network. I added the network option in there and seems like network id and site id (blog id) are available at the point, what is the issuing in saving it?

#20 in reply to: ↑ 19 @flixos90
4 weeks ago

Replying to spacedmonkey:

  1. To cache the result. The main site will likely never change. Because of this fact, it should be stored. This saves doing any queries on the blogs table. Take for example a network with 25k sites and 57 networks and no object cache enabled. That get_sites lookup could be expensive. Calling from a network option is a much faster lookup.

Right, that's a good point. With the latest patch 29684.5.diff the lookup is only cached for sites with an external object cache. A network option would indeed improve this.

  1. Storing it in the network option makes the lookup much easier. How easy is it to find list of a main sites?

I don't necessarily think using a network option makes it easier than simply using get_main_site_id().

I am not sure I understand your point about populate_network. I added the network option in there and seems like network id and site id (blog id) are available at the point, what is the issuing in saving it?

Please review your latest patch 29684.4.diff: It only sets the main_site for the initial network that is set up. The network option is not set for any further network (like any other network when using multi-network). It actually does not work through that function since it is unaware of the matching site ID (except for when setting up multisite, where it's always 1).

Summarizing, I'm not convinced we need a network option. I'm also not heavily against it though. Let's wait for further opinions.

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


3 weeks ago

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


2 weeks ago

Note: See TracTickets for help on using tickets.