WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37414 closed enhancement (fixed)

Use `get_network()` instead of global `$current_site`

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

Description

get_network() falls back to the current network if no parameter is provided. That means we can use it anywhere we currently work with globals or with get_current_site(). I think the latter function should be deprecated afterwards.

Attachments (3)

37414.diff (25.6 KB) - added by flixos90 3 years ago.
37414.2.diff (35.6 KB) - added by flixos90 3 years ago.
37414.3.diff (35.5 KB) - added by jeremyfelt 3 years ago.

Download all attachments as: .zip

Change History (20)

#1 @flixos90
3 years ago

  • Milestone changed from Future Release to 4.7
  • Owner set to flixos90
  • Status changed from new to assigned

#3 @flixos90
3 years ago

@wonderboymusic: Seeing the changeset above, not sure if that's the right step as we should start to use get_network() instead of get_current_site() or the global $current_site. Maybe we should make the new functions get_site() and get_network() compatible with non-multisite and then move these too.

@flixos90
3 years ago

#4 @flixos90
3 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

In 37414.diff I replaced all instances of get_current_site() with get_network(), plus some related adjustments, like changing internal variable names from $current_site to $current_network.

While get_network() is not available on some of these functions, this should not be a problem since there are is_multisite() checks in place that prevent it from being called. This was similar with get_current_site() as well, so I was not sure why the function was moved in 38388. The global $current_site is not available on non-multisite, so I think that function should remain in an ms-*.php file.

#5 follow-up: @jeremyfelt
3 years ago

  • Keywords needs-refresh added; dev-feedback removed

[38388] has been reverted in [38636]. Due to this and possibly some other churn, 37414.diff is having some trouble applying cleanly.

I like the idea, especially from a naming perspective. This change would also provide the filter in places where it wasn't available before, which is probably good. It does mean that apply_filters() fires that many more times in a page view, though I have not studied to see if there's a real enough performance impact because of that.

It could be good to do some profiling, but this seems like a nice change.

#6 in reply to: ↑ 5 ; follow-up: @flixos90
3 years ago

Replying to jeremyfelt:

[38388] has been reverted in [38636]. Due to this and possibly some other churn, 37414.diff is having some trouble applying cleanly.

Let's see if we can get #34450 in first as this would contain another case that this ticket would apply to. I can take care of creating an updated patch afterwards.

#7 in reply to: ↑ 6 @jeremyfelt
3 years ago

Replying to flixos90:

Replying to jeremyfelt:

[38388] has been reverted in [38636]. Due to this and possibly some other churn, 37414.diff is having some trouble applying cleanly.

Let's see if we can get #34450 in first as this would contain another case that this ticket would apply to. I can take care of creating an updated patch afterwards.

And #34450 is in. :)

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


3 years ago

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


3 years ago

@flixos90
3 years ago

#10 @flixos90
3 years ago

  • Keywords needs-refresh removed

37414.2.diff is a new patch that works against current trunk. Other than that, it does the same as mentioned above in https://core.trac.wordpress.org/ticket/37414#comment:4.

A site note: While doing the replacements, I found two minor issues that are a bit related, #38246 and #38247. Maybe those should be merged before this one.

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


3 years ago

#12 @jeremyfelt
3 years ago

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

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


3 years ago

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


3 years ago

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


3 years ago

@jeremyfelt
3 years ago

#16 @jeremyfelt
3 years ago

37414.3.diff is another refresh.

Things are looking good.

  • I did some brief performance testing between the global $current_site() and get_network() and things are only slightly slower. In most cases, this will be pulling from something that's already available. The only real overhead is the application of the get_network filter on every usage, which is a nice side effect on its own.
  • All unit tests in single and multisite are passing. We aren't using get_network() or get_current_network_id() in any place that is not multisite.
  • Single site install, multisite setup, multisite management, new site signup, are all working as expected. This covers some of the things that are kind of hard to cover in our tests.

Let's do it. :)

#17 @jeremyfelt
3 years ago

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

In 38814:

Multisite: Use get_network() and get_current_network_id() for current network data.

get_network() falls back to the current network when called without any arguments. Between this and get_current_network_id(), we can replace almost all instances of the global $current_site and all instances of get_current_site().

This effectively deprecates get_current_site(), something that we'll do in a future ticket.

Props flixos90.
Fixes #37414.

Note: See TracTickets for help on using tickets.