WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 5 weeks ago

Last modified 2 days ago

#29684 closed enhancement (fixed)

Add get_main_site_id() function

Reported by: rmccue Owned by: flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.9
Component: Networks and Sites Keywords: has-patch, has-unit-tests, ms-roadmap
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 (18)

29684.diff (2.4 KB) - added by jeremyfelt 3 years ago.
29684.2.diff (6.2 KB) - added by flixos90 3 months ago.
29684.3.diff (2.0 KB) - added by spacedmonkey 3 months ago.
29684.4.diff (2.7 KB) - added by spacedmonkey 3 months ago.
29684.5.diff (7.9 KB) - added by flixos90 3 months ago.
29684.6.diff (6.3 KB) - added by jeremyfelt 8 weeks ago.
29684.7.diff (10.2 KB) - added by jeremyfelt 8 weeks ago.
29684.8.diff (6.3 KB) - added by spacedmonkey 8 weeks ago.
29684.9.diff (14.1 KB) - added by spacedmonkey 8 weeks ago.
29684.10.diff (9.6 KB) - added by flixos90 7 weeks ago.
29684.11.diff (6.5 KB) - added by spacedmonkey 7 weeks ago.
29684.12.diff (9.6 KB) - added by spacedmonkey 7 weeks ago.
29684.13.diff (9.6 KB) - added by spacedmonkey 7 weeks ago.
29684.14.diff (13.4 KB) - added by spacedmonkey 7 weeks ago.
29684.15.diff (8.7 KB) - added by flixos90 6 weeks ago.
29684.16.diff (8.8 KB) - added by flixos90 6 weeks ago.
29684.17.diff (19.2 KB) - added by spacedmonkey 6 weeks ago.
29684.18.diff (11.3 KB) - added by flixos90 5 weeks ago.

Download all attachments as: .zip

Change History (67)

@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
23 months ago

  • Keywords needs-unit-tests added

#3 @jeremyfelt
23 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.


20 months ago

#5 @jeremyfelt
20 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.


20 months ago

#7 @jorbin
20 months ago

  • Milestone changed from 4.5 to Future Release

#8 @flixos90
4 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
3 months ago

#9 @flixos90
3 months 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.


3 months ago

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


3 months ago

#12 @flixos90
3 months 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
3 months 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.

#14 @spacedmonkey
3 months 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
3 months 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
3 months 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
3 months ago

#17 @flixos90
3 months 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
3 months ago

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

#19 follow-up: @spacedmonkey
3 months 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
3 months 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 months ago

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


2 months ago

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


8 weeks ago

@jeremyfelt
8 weeks ago

@jeremyfelt
8 weeks ago

#24 @jeremyfelt
8 weeks ago

We discussed this ticket during bug scrub quite a bit today. In the first iteration we're going to plan on sticking close to what's being pulled from the current bootstrap and not introduce a new network option. Once this is in, and possibly once network meta is available, we may be able to adjust.

I've created a new tests file getMainSiteId.php in 29684.7.diff that uses a common factory for the networks and sites and separates these tests from the standard site tests.

There is one failing test - When using get_main_site_id() without specifying the network when switched to a site that is not on the current network, the current network is used rather than the switched site's network.

I tried using get_site() to retrieve the network ID in this case, but other unit tests blew up. There's a good chance I was missing something simple, but I'm going to upload the patch with the failing test as is.

@spacedmonkey
8 weeks ago

@spacedmonkey
8 weeks ago

#25 follow-up: @spacedmonkey
8 weeks ago

  • Keywords ms-roadmap added

29684.9.diff

Key changes.

  • Move get_main_site_id filter to top of function for better short circuit.
  • Check to see if blog_id is set on network object.

#26 in reply to: ↑ 25 @jeremyfelt
7 weeks ago

Replying to spacedmonkey:

29684.9.diff

Key changes.

  • Move get_main_site_id filter to top of function for better short circuit.

If we move the filter to the top, we should probably name it pre_get_main_site_id to clarify that it's a short circuit. The docs will need to be moved in the patch as well.

I've been going back and forth on whether we needed a filter at all. I could see the short circuit being more valuable than one at the end.

  • Check to see if blog_id is set on network object.

Unfortunately, we can't do this because if blog_id is not set, it's populated by get_main_site_id(). As is, the patch causes a segfault in PHP.

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


7 weeks ago

#28 follow-up: @flixos90
7 weeks ago

29684.10.diff changes the following:

  • Rename the filter to pre_get_main_site_id() and adjust the docs accordingly to indicate it's a short-circuiting filter.
  • Remove the check for whether $network->blog_id is set.
  • Remove two tests (including the failing one) that actually weren't really useful for testing the function since they actually would need to rely on network switching to have a purpose (which we don't have in core).

@flixos90
7 weeks ago

#29 in reply to: ↑ 28 @jeremyfelt
7 weeks ago

Replying to flixos90:

29684.10.diff changes the following:

  • Rename the filter to pre_get_main_site_id() and adjust the docs accordingly to indicate it's a short-circuiting filter.

+1

  • Remove two tests (including the failing one) that actually weren't really useful for testing the function since they actually would need to rely on network switching to have a purpose (which we don't have in core).

These tests are both real world conditions that I'd expect the function as written to handle. One returns data as expected, one highlights an area of confusion if we don't address it.

I have a feeling that get_site() will work (by providing the site's network ID) and that there's an unrelated issue in another test that's causing cross-pollution.

#30 @spacedmonkey
7 weeks ago

29684.13.diff

Small change, changed back last line of is_main_site to the following.

return (int) $site_id === (int) get_network( $network_id )->site_id; 

Because there is a magic getting in WP_Network, calling site_id will call get_main_site_id. The reason you want to make sure the magic getter run, is that it only runs once. This stops the logic running multiple times.

#31 @flixos90
7 weeks ago

@jeremyfelt

I have a feeling that get_site() will work (by providing the site's network ID) and that there's an unrelated issue in another test that's causing cross-pollution.

Ah right, I missed this. Let's add these two tests back in, and in get_main_site_id() call get_network( get_site()->network_id ). A bit strange, but that should make it work.

@spacedmonkey

Because there is a magic getting in WP_Network, calling site_id will call get_main_site_id. The reason you want to make sure the magic getter run, is that it only runs once. This stops the logic running multiple times.

I think I prefer to call regular functions from other functions instead of relying on object properties. Furthermore this will only have benefits if $network_id is null, because otherwise the network returned is a new object and it must call get_main_site_id() regardless. I'm not heavily opposed, but I lean towards keeping get_main_site_id() in there.

Last edited 7 weeks ago by flixos90 (previous) (diff)

#32 @spacedmonkey
7 weeks ago

99% of the time (even in a multi network setup) that this function will be called, network_id will be passed as null. Meaning you are really referencing $current_site, setup in the bootstrap. In all my setups, the $current_site is setup in the sunrise file. We should be reference the class property here, not the function. Get main site id should only ever be called as failsafe if that property isn't set.

#33 follow-up: @spacedmonkey
7 weeks ago

29684.14.diff

Is a big refactor of the code. Key points.

  • Moved logic from get_main_site_id to get_main_site_id method on WP_Network. This has a number of benefits. It keeps logic related to the WP_Network class, in it's class. You can also check to see if blog_id is already set before doing anything. It can also just reference other class level properties really easily.
  • Only run the logic once and just reference blog_id going forward. This is a performance win, as stop code running unnecessarily.
  • In ms-load.php I removed repeated logic. It is not required, as get_main_site_id method will do it now.
  • In ms-load.php pass the object instead of the id into get_main_site_id. This just force the getter to run making sure that blog_id is already set.

The key point about this change is the blog_id should be seen as truth, not the function. If you setup your own $current_site with your own logic in the sunrise, you should never need this code.

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


6 weeks ago

#35 @jeremyfelt
6 weeks ago

#41807 was marked as a duplicate.

#36 @spacedmonkey
6 weeks ago

I have been discussing this with @schlessera related to this issue with WP-CLI. Basically WP-CLI does currently allow you to delete main site on a network. WP CLI introducts some weird edge cases. Like

  • You delete main site network using WP-CLI.
  • You edit main site's domain / path to not match the main site. This would be mean the match the network domain.

Storing it network option allows you to easily change the main site, fixing the above issues by creating a new site and making it main site in options. You can already change the main site on a network using the constants, but this isn't always as easy to do in a multi-network setups. You also get benefits like

  • Easy cache invalidation and management
  • Main site can be primed in memory, by adding key to wp_load_core_site_options
  • On sites without object caching, it saves a query to the blogs table and all the overhead that brings.
  • Easy to find main site using WP CLI or other tools by just looking in database.

I don't understand why it would just be a network option, if we are happy with caching the result of the get_sites call, then why not just store it forever...

#37 in reply to: ↑ 33 ; follow-up: @flixos90
6 weeks ago

@spacedmonkey

Moved logic from get_main_site_id to get_main_site_id method on WP_Network. This has a number of benefits. It keeps logic related to the WP_Network class, in it's class. You can also check to see if blog_id is already set before doing anything. It can also just reference other class level properties really easily.

Putting the logic for get_main_site_id() into WP_Network causes the pre_get_main_site_id filter not being run everytime, which I would expect when calling the function directly. You can see that by the respective test failing in 29684.14.diff. Furthermore this contradicts with other patterns used in similar cases: In most cases in core, functions are the canonical "source of truth", and class methods just wrap around those. For those reasons, I prefer the following approach:

  • If you call get_main_site_id() explicitly, the logic always runs, filter fires etc. This shouldn't introduce performance issues since the result is cached anyway.
  • The tweak in WP_Network should only be there for (rather rare) cases someone wants to access $network->site_id. This is not a common practice at all, especially since it doesn't even work now except for the current network set in the bootstrap process.
  • If we use the $current_site global in get_main_site_id() at all, it should still run the filter before and only use the property value if the function wasn't short-circuited.

Regarding passing a network object instead of ID to get_main_site_id(), that's worth looking into. In ms-load.php however it doesn't make sense, since that code is only run if the property is not set anyway. While looking at this, I also noticed that the if ( empty( $current_site->blog_id ) ) clause alone may already fill the property due to the "magic logic". That doesn't really have any effects, but is sub-optimal.

@jeremyfelt

These tests are both real world conditions that I'd expect the function as written to handle. One returns data as expected, one highlights an area of confusion if we don't address it.

I think we should leave the tests for switching to a site in another network out as we cannot expect get_main_site_id() to know the current network correctly, as there is no network switching in core. If that is fixed at some point, get_network() should be responsible for making sure we get the current network (the network of the site that has been switched to). There are countless occasions of this behavior in core (due to the lack of network switching), therefore I'd prefer to ignore this here.


All these thoughts and concerns so far make me think that we should for the first iteration simply introduce get_main_site_id(). Not make changes to WP_Network at all and investigate that in a further ticket, similar to looking at whether we need a network option or not.

@flixos90
6 weeks ago

#38 @flixos90
6 weeks ago

29684.15.diff is a simplified patch solely focussing on get_main_site_id() itself, as suggested in my last comment. It makes the following changes to the function:

  • Run get_network() in the very beginning of the function. If no network found, the ID passed is invalid, so the function should return 0 then regardless of the filter. The main reason for running get_network() earlier is so that the pre_get_main_site_id is always passed an actual ID and never null (this was previously possible).
  • The Tests_Multisite_Get_Main_Site_ID class now is properly wrapped in an is_multisite() checked and contains a docblock with the respective test groups.

29684.16.diff is a slightly modified version: The only change is that get_main_site_id() uses the $site_id property of WP_Network if it is set. Not entirely sure if we should do that for the first iteration since it interacts with WP_Network, but I like the idea and it would address some of the concerns @spacedmonkey was having.

Last edited 6 weeks ago by flixos90 (previous) (diff)

@flixos90
6 weeks ago

#39 @spacedmonkey
6 weeks ago

29684.16.diff - Feels extremely limiting and half a solution. This is patch, it really does feel like we are pulling at threads, as every time we think have it, something else comes up.

We now have two options, either a limited patch in the form of 29684.16.diff or wider reacting patch of 29684.17.diff. 29684.17.diff an fixes an issue for me, as the site_id is not automatically setup when referencing a different network. I don't like having a class level property that I can't trust that exists. It makes it useless to me. The magic getting forces it be set. This will likely fixes issues with multi network setups that we don't even know exist.

In work in the networks endpoint I list networks. Without the magic logic, all but the current network have site id set to 0. Even if I do this

get_network(1)->site_id

it will return 0. This really fixes broken and is confusing that to other developer that don't understand the history of this.

#40 in reply to: ↑ 37 @jeremyfelt
6 weeks ago

Replying to flixos90:

I think we should leave the tests for switching to a site in another network out as we cannot expect get_main_site_id() to know the current network correctly, as there is no network switching in core.

That's what makes a lot of this uglier than it should be. We do support switching between sites that are on different networks. If we can't reliably provide data in those cases, then we shouldn't provide something until it's possible.

What if we step back to the beginning and think about a get_main_site_id() that does not support a $network_id parameter? It should only provide the main site for the current network.

Is that useful enough to be introduced? If it is, I think that can happen without storing a network option and would simplify all of this significantly.

#41 follow-up: @johnjamesjacoby
6 weeks ago

WP Multi Network has the following function names, which do exactly as they say:

  • get_main_site_for_network()
  • is_main_site_for_network()

I prefer these over get_main_site_id() which, similar to get_main_network_id() insinuates that it's the single-one-and-only primary main omega site ID for the entire installation, not for any specific network. Put another way, it's the main site for the main network, and that's it. If you want the main site for a network, use the functions above.

Probably not ironically, those functions in the WP Multi Network plugin are almost identical to the patches attached here. (There's only really so many ways to do this.) The major difference being that WPMN doesn't check the SITE_ID_CURRENT_SITE type globals here, because it really doesn't work very well to define those globals in a multi-network arrangement.

#42 in reply to: ↑ 41 @flixos90
5 weeks ago

Replying to johnjamesjacoby:

  • get_main_site_for_network()
  • is_main_site_for_network()

I prefer these over get_main_site_id() which, similar to get_main_network_id() insinuates that it's the single-one-and-only primary main omega site ID for the entire installation, not for any specific network. Put another way, it's the main site for the main network, and that's it. If you want the main site for a network, use the functions above.

I can see these names being more precise than what the patches so far do. A possible issue I see is that is_main_site() already exists. We could of course introduce get_main_site_for_network() and is_main_site_for_network() (or, another idea, get_network_main_site() and is_network_main_site()) and then defer is_main_site() to the new function, passing the ID of the $current_site global as $network_id parameter.

I personally don't think the names we've been working with so far are really worse. In a regular multisite, they feel more straightforward, as the "which network is it?" thing only matters for multi-network. My take is for the most part that we should support multi-network as good as possible, but still focus on common multisite for things like that.

An interesting observation is that core so far (what a surprise) doesn't have a standard on this:

  • We have get_user_count() and get_blog_count() which should rather be called get_network_user_count() (yeah, it's a global count, but it's still a network option) and get_network_site_count() ("site" only because "blog" is obsolete) if following your suggestion.
  • Other functions like wp_update_network_user_counts() or wp_update_network_site_counts() are following your suggestion.
  • Something to consider is that the functions discussed here would be available on non-multisite as well. Having a term like "network" in the function name may feel weird for these setups.

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


5 weeks ago

@flixos90
5 weeks ago

#44 @flixos90
5 weeks ago

  • Owner changed from jeremyfelt to flixos90

29684.18.diff is a mixture between 29684.15.diff and 29684.17.diff, as discussed on Tuesday. It contains get_main_site_id() as a standalone function, magic getters for the WP_Network::$blog_id and WP_Network::$site_id properties (those are the same, except for their type), and the get_main_site_id() function is used in the multisite bootstrap process to fill the $blog_id property if not yet set.

The patch furthermore makes the following minor tweaks over the previous patches:

  • Ensure the property types remain backward-compatible. WP_Network::$blog_id is expected to be a string, only the virtual WP_Network::$site_id property must return an integer. Therefore the helper method WP_Network::get_main_site_id() always returns a string.
  • Add an extra test in the network tests class to ensure the $blog_id property is always set correctly.
  • Improve the tests in the site tests class by using existing network/site objects created in wpSetUpBeforeClass().

#45 @flixos90
5 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 41380:

Multisite: Introduce get_main_site_id().

This function can be used to easily get the main site ID of a given network via the optional $network_id parameter, which defaults to the current network. The existing is_main_site() now uses the new function internally and now accepts an optional $network_id parameter as well.

The main purpose of the new function at this point is to ensure that the WP_Network::$blog_id property is always set. Magic getters in the class have been adjusted to auto-fill the property when it is accessed and empty. Furthermore the function encapsulates logic that was previously part of ms_load_current_site_and_network() and has been replaced with a call to the function now.

Props spacedmonkey, jeremyfelt, johnjamesjacoby, flixos90.
Fixes #29684.

#46 @spacedmonkey
5 weeks ago

Looking at 41380 there are a number of issues / things I would change.

  • In is_main_site do not use get_main_site_id. It you have built a WP_Network and set $current_site this will be ignored and logic run again. Return to int) get_network($network_id)->site_id;
  • Change docs / variable name for is_main_site and get_main_site_id. $network_id is not valid docs, as it allows, int, WP_Network, object and null to be passed.
  • Where possible we should as the object to get_main_site_id. Example $current_site->blog_id = get_main_site_id( $current_site->id ); can be changed to $current_site->blog_id = get_main_site_id( $current_site ); . This saves needlessly generating new a WP_Network object.
  • Change pre_get_main_site_id to pass network object as second param.

cc @flixos90

#47 @flixos90
5 weeks ago

  • Keywords needs-dev-note added

@spacedmonkey Let's discuss these things in separate follow-up tickets, I'd propose one for passing network objects and another one for possibly using the WP_Network::$blog_id property in is_main_site().

I'm adding the needs-dev-note keyword since the function, filter and implications for WP_Network::$blog_id should probably be mentioned in our 4.9 note.

#49 @jeremyfelt
2 days ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.