WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#33900 closed enhancement (fixed)

introduce get_current_network_id helper function

Reported by: spacedmonkey Owned by: flixos90
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: Networks and Sites Keywords: has-patch
Focuses: multisite Cc:

Description

Similar to get_current_blog_id helper function. This function can be used to get the id of the current_site global.

Attachments (5)

33900.patch (506 bytes) - added by spacedmonkey 4 years ago.
33900.2.patch (610 bytes) - added by spacedmonkey 4 years ago.
33900.3.patch (2.6 KB) - added by flixos90 3 years ago.
33900.diff (1.7 KB) - added by jeremyfelt 3 years ago.
33900.2.diff (1.4 KB) - added by jeremyfelt 3 years ago.

Download all attachments as: .zip

Change History (22)

@spacedmonkey
4 years ago

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


4 years ago

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


4 years ago

#3 @johnjamesjacoby
4 years ago

The $current_blog variable is only created in multisite in ms-settings.php (and maybe sunrise.php).

  • Would this benefit from an !empty() check?
  • Should this function live outside of multisite, maybe in load.php next to get_current_blog_id()?

#4 @spacedmonkey
4 years ago

Did the feedback from @johnjamesjacoby . Did one better than check if empty and did a type check.

#5 @flixos90
3 years ago

  • Keywords has-patch added

#6 @jeremyfelt
3 years ago

  • Milestone changed from Awaiting Review to 4.6

I've looked for this a couple times recently. We should go for it. :)

#7 @jeremyfelt
3 years ago

In single site, get_current_blog_id() will return 1 because the global $blog_id is setup in wp_initial_constants(). We don't do anything similar for a global network ID, though in get_main_network_id() we return 1 if not multisite.

So I guess my question is, does this logic make sense:

  • If ! is_multisite(), return 1.
  • If is_multisite() and not a WP_Network instance, return 1.
  • If is_multisite() and a WP_Network instance, return ID.

What would lead to the second condition?

#8 follow-up: @johnjamesjacoby
3 years ago

My thoughts:

  • Yes
  • Yes
  • Yes

The second condition could maybe happen if sunrise.php was already in place and wasn't using core functions to populate $current_site, and honestly this seems likely for anyone that's using sunrise.php since the only thing it is really great at is pre-populating those globals.


Tangentially, the assumed 1 main network ID return value bothers me. A new default constant makes sense here, itself defaulted to 1 in WordPress core. And it can still be overridden in sunrise.php or wp-config.php. It's edge-case, but I've already needed to kludge this because a new network with a few sites took the place of the original one.

So for each of the 3 cases, I'd like for the 1 default return value to be configurable in some way. That could be handled in it's own ticket, too, since it's a separate initiative and could cascade into the default $blog_id too?

Last edited 3 years ago by johnjamesjacoby (previous) (diff)

#9 in reply to: ↑ 8 @jeremyfelt
3 years ago

Replying to johnjamesjacoby:

The second condition could maybe happen if sunrise.php was already in place and wasn't using core functions to populate $current_site, and honestly this seems likely for anyone that's using sunrise.php since the only thing it is really great at is pre-populating those globals.

This should be mitigated by the check and re-assignment at the end of ms-settings.php. So it seems like any real use of get_current_network_id() by an mu-plugin, plugin, or theme would be after those objects are properly available.

Tangentially, the assumed 1 main network ID return value bothers me. A new default constant makes sense here, itself defaulted to 1 in WordPress core. And it can still be overridden in sunrise.php or wp-config.php. It's edge-case, but I've already needed to kludge this because a new network with a few sites took the place of the original one.

So for each of the 3 cases, I'd like for the 1 default return value to be configurable in some way. That could be handled in it's own ticket, too, since it's a separate initiative and could cascade into the default $blog_id too?

Should the default used in get_current_network_id() be based on get_main_network_id()? We use the PRIMARY_NETWORK_ID (oops) constant and get_main_network_id filters there that could be used to override. That seems to be a safe assumption— main == default.

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


3 years ago

#11 @chriscct7
3 years ago

  • Owner set to flixos90
  • Status changed from new to assigned

@flixos90
3 years ago

#12 follow-up: @flixos90
3 years ago

33900.3.patch is an updated patch with the following changes:

  • return 1 if no multisite
  • use get_main_network_id() if $current_site is not a WP_Network instance
  • add a similar instance check in get_main_network_id() (otherwise this change would have no point and could cause an error)
  • make sure to return an integer (the id property of WP_Network is a string)

I wonder whether the instance check is really necessary (edge-case). We can probably do it since it shouldn't cause issues, but we don't do a check like this in other areas of Core (see the change in get_main_network_id() in this patch for example). I wonder whether this is worth changing in this function - I'm not opposed to it, it's just different from how we handle $current_site in the rest of Core (always assume it's a WP_Network).

Btw sorry about the weird spacing changes in wp-includes/load.php, not sure what my editor did there.

Last edited 3 years ago by flixos90 (previous) (diff)

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


3 years ago

@jeremyfelt
3 years ago

#14 in reply to: ↑ 12 ; follow-up: @jeremyfelt
3 years ago

Replying to flixos90:

I wonder whether the instance check is really necessary (edge-case)

We may be better off with an isset() instead. If the function is used in sunrise.php for some reason, it's possible that things are not full objects yet but still have the id property. We then convert to WP_Network object in ms-settings.php. 33900.diff takes this route.

Thoughts?

#15 in reply to: ↑ 14 @flixos90
3 years ago

Replying to jeremyfelt:

We may be better off with an isset() instead. If the function is used in sunrise.php for some reason, it's possible that things are not full objects yet but still have the id property. We then convert to WP_Network object in ms-settings.php. 33900.diff takes this route.

Thoughts?

Makes sense to me, this will make it more accurate in those use-cases.

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


3 years ago

@jeremyfelt
3 years ago

#17 @jeremyfelt
3 years ago

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

In 37670:

Multisite: Introduce get_current_network_id()

Similar to get_current_blog_id, this can be used to get the ID of the $current_site global. If not available, it will fallback to the main network ID. In single site, this will return 1.

Props spacedmonkey, flixos90.
Fixes #33900.

Note: See TracTickets for help on using tickets.