WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#25158 closed defect (bug) (fixed)

Use get_current_site() instead of the $current_site global when possible

Reported by: jeremyfelt Owned by: SergeyBiryukov
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Multisite Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

Via prod from Nacin, this does seem like a good cleanup opportunity

Patch removes almost all uses of the global $current_site and replaces with get_current_site().

Notes:

  1. is_main_site() doesn't necessarily have access to get_current_site() when first used and therefore requires the global $current_site.
  2. populate_network() sets parts of the $current_site global when transitioning from single site to multisite and therefore needs access.
  3. get_current_site() obviously needs access to the global
  4. get_current_site_name() uses a local $current_siteobject that is passed to it
  5. wpmu_current_site() is responsible for setting up the $current_site global and therefore appears to need access. However, I'm not entirely convinced it really is required as wpmu_current_site() is only called once by core and that is to set the global in ms-settings.php.
  6. ms-settings.php is where $current_site hits the global scope, so it stays.

No new failures arise in tests for both single and multisite mode. There were a couple places where I made small code formatting tweaks, though I tried to restrain. :)

Attachments (6)

25158.diff (37.1 KB) - added by jeremyfelt 6 years ago.
25158.2.diff (20.1 KB) - added by jeremyfelt 6 years ago.
25158.3.diff (18.8 KB) - added by jeremyfelt 6 years ago.
25158.4.diff (18.8 KB) - added by SergeyBiryukov 6 years ago.
25158.5.diff (1.3 KB) - added by SergeyBiryukov 6 years ago.
25158.6.diff (1.7 KB) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (24)

@jeremyfelt
6 years ago

#1 follow-up: @nacin
6 years ago

If a function is repeatedly calling get_current_site(), one option is to also assign get_current_site() to a local variable $current_site, and remove the globalization, at which point you won't need to change the function otherwise. This is how we resolved using get_post() instead of the global $post without rewriting dozens of $post references.

#2 in reply to: ↑ 1 @jeremyfelt
6 years ago

Replying to nacin:

If a function is repeatedly calling get_current_site(), one option is to also assign get_current_site() to a local variable $current_site

Yeah, I started down that road, but then went all in. There are a few places where that would likely make sense though.

@jeremyfelt
6 years ago

#3 follow-up: @jeremyfelt
6 years ago

25158.2.diff makes use of a local $current_site = get_current_site(); when it makes sense rather than calling get_current_site() each time. There are a couple places where I've left multiple function calls in place as there are situations where only one of them will be called depending on logic.

Also removed an unused $wpdb global in ms_site_check()

#4 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
6 years ago

Replying to jeremyfelt:

25158.2.diff makes use of a local $current_site = get_current_site(); when it makes sense rather than calling get_current_site() each time.

I'd suggest doing the same in all the instances. It seems more future-proof in case more instances are added later in the same function.

#5 in reply to: ↑ 4 @nacin
6 years ago

Replying to SergeyBiryukov:

I'd suggest doing the same in all the instances. It seems more future-proof in case more instances are added later in the same function.

I think this is a judgment call; sometimes it may make sense, sometimes it is just an unnecessary variable. Really no different than when we were replacing $current_screen with get_current_screen() or $post with get_post().

#6 @SergeyBiryukov
6 years ago

Indeed. I would probably keep the variable in network_site_url() and network_home_url(), where it's used twice in the same line. Otherwise, 25158.2.diff looks good to me.

@jeremyfelt
6 years ago

#7 @jeremyfelt
6 years ago

25158.3.diff addresses the changes suggested by Sergey and refreshes against current trunk.

#8 @SergeyBiryukov
6 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.7

#9 follow-up: @SergeyBiryukov
6 years ago

25158.4.diff keeps the variable in wpmu_delete_blog(), where it's also used twice in the same line.

We should probably keep the global in admin-header.php, see [18526] and #16143.

#10 in reply to: ↑ 9 @SergeyBiryukov
6 years ago

Replying to SergeyBiryukov:

We should probably keep the global in admin-header.php, see [18526] and #16143.

On second thought, removing the global seems fine.

#11 @nacin
6 years ago

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

Sergey said he was going to go through this.

#12 @nacin
6 years ago

  • Milestone changed from 3.7 to 3.8

#13 @SergeyBiryukov
6 years ago

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

In 26120:

Use get_current_site() instead of the $current_site global when possible.

props jeremyfelt.
fixes #25158.

#14 @SergeyBiryukov
6 years ago

In 26124:

Restore the $current_site global in wp-admin/admin-header.php to avoid an undefined function error in single site.

see #25158.

#15 @SergeyBiryukov
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

25158.5.diff reverts [26124] and checks for is_multisite() before calling get_current_site(). Also removes the global from is_main_network().

#16 @SergeyBiryukov
6 years ago

Looks like we don't actually need the variable in wp-admin/admin-header.php and can replace it with get_current_site() there too: 25158.6.diff.

#17 @SergeyBiryukov
6 years ago

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

In 26235:

Use get_current_site() instead of the $current_site global in wp-admin/admin-header.php and is_main_network().

fixes #25158.

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


4 years ago

Note: See TracTickets for help on using tickets.