[wp-trac] [WordPress Trac] #29684: Add get_main_site_id() function

[WordPress Trac]

#29684: Add get_main_site_id() function
-------------------------------------------------+-------------------------
 Reporter:  rmccue                               |       Owner:  jeremyfelt
     Type:  enhancement                          |      Status:  reviewing
 Priority:  normal                               |   Milestone:  4.9
Component:  Networks and Sites                   |     Version:  3.9
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-unit-tests ms-roadmap  |     Focuses:  multisite
-------------------------------------------------+-------------------------

Comment (by flixos90):

 @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 [attachment: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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/29684#comment:37>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform