Opened 10 years ago
Closed 7 years 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 (19)
Change History (71)
#1
@
10 years ago
- Keywords dev-feedback has-patch added
- Milestone changed from Awaiting Review to Future Release
- Type changed from defect (bug) to enhancement
#3
@
9 years 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.
9 years ago
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#8
@
7 years 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 )
.
#9
@
7 years 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 usesWP_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 useget_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 toget_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.
7 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
#12
@
7 years 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
@
7 years 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
@
7 years 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:
↓ 16
@
7 years 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
@
7 years 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 ofget_main_site()
so that it can be called independently. The magic getter could then callget_main_site()
.
#17
@
7 years 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 withget_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 ofms_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 newget_main_site_id()
to get its result. - The bottom of
ms_load_current_site_and_network()
is now simplied since it simply needs to callget_main_site_id()
. This also makes it simpler for authors of customsunrise.php
files. WP_Network
now includes a privateget_main_site_id()
method that is called by the magic getters forblog_id
/site_id
(as a reminder, these affect the same value). The method looks at the actual property and if it is not set callsget_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 relatedis_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
@
7 years ago
- Owner changed from flixos90 to jeremyfelt
- Status changed from assigned to reviewing
#19
follow-up:
↓ 20
@
7 years ago
Saving in in network options, there were a couple of reason.
- 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.
- 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.
- Storing it in the network option makes the lookup much easier. How easy is it to find list of a main sites?
- 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
@
7 years ago
Replying to spacedmonkey:
- 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.
- 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.
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
#24
@
7 years 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.
#25
follow-up:
↓ 26
@
7 years ago
- Keywords ms-roadmap added
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
@
7 years ago
Replying to spacedmonkey:
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 years ago
#28
follow-up:
↓ 29
@
7 years 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).
#29
in reply to:
↑ 28
@
7 years 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
@
7 years ago
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
@
7 years 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.
#32
@
7 years 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:
↓ 37
@
7 years ago
Is a big refactor of the code. Key points.
- Moved logic from
get_main_site_id
toget_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 ifblog_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.
7 years ago
#36
@
7 years 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:
↓ 40
@
7 years ago
@spacedmonkey
Moved logic from
get_main_site_id
toget_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 ifblog_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 inget_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.
#38
@
7 years 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 return0
then regardless of the filter. The main reason for runningget_network()
earlier is so that thepre_get_main_site_id
is always passed an actual ID and nevernull
(this was previously possible). - The
Tests_Multisite_Get_Main_Site_ID
class now is properly wrapped in anis_multisite()
checked and contains a docblock with the respective test groups.
#39
@
7 years 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
@
7 years 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:
↓ 42
@
7 years 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
@
7 years ago
Replying to johnjamesjacoby:
get_main_site_for_network()
is_main_site_for_network()
I prefer these over
get_main_site_id()
which, similar toget_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()
andget_blog_count()
which should rather be calledget_network_user_count()
(yeah, it's a global count, but it's still a network option) andget_network_site_count()
("site" only because "blog" is obsolete) if following your suggestion. - Other functions like
wp_update_network_user_counts()
orwp_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.
7 years ago
#44
@
7 years 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 virtualWP_Network::$site_id
property must return an integer. Therefore the helper methodWP_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()
.
#46
@
7 years 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 aWP_Network
and set $current_site this will be ignored and logic run again. Return toint) get_network($network_id)->site_id;
- Change docs / variable name for
is_main_site
andget_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
@
7 years 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.
#50
@
7 years ago
- Keywords changed from has-patch, has-unit-tests, ms-roadmap to has-patch has-unit-tests ms-roadmap
- Resolution fixed deleted
- Status changed from closed to reopened
Per discussion in today's [multisite ticket scrub](https://wordpress.slack.com/archives/C03BVB47S/p1509383352000072), the network object should be passed to the pre_get_main_site_id
filter instead of just the ID. This was previously discussed already, but somewhat overlooked.
It's a late fix, but simple enough to change it before RC.
#51
@
7 years ago
29684.19.diff passes the network object to pre_get_main_site_id
and includes a test where the filter is used to change the main site ID of a specific network.
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 beget_main_site_id()
.The current bootstrap tests still pass, though we would need something more comprehensive for this function.