WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#37823 closed enhancement (fixed)

Use get_sites in Network upgrade

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

Description

Instead of using raw sql query in upgrade.php, use the new get_sites function.

Attachments (5)

37823.diff (834 bytes) - added by spacedmonkey 3 years ago.
37823.2.diff (1.5 KB) - added by spacedmonkey 3 years ago.
37823.3.diff (1.5 KB) - added by spacedmonkey 3 years ago.
37823.4.diff (1.5 KB) - added by spacedmonkey 3 years ago.
37823.5.diff (1.7 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (17)

@spacedmonkey
3 years ago

@spacedmonkey
3 years ago

@spacedmonkey
3 years ago

#1 @spacedmonkey
3 years ago

  • Keywords has-patch needs-unit-tests added

#2 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 4.7

#3 @flixos90
3 years ago

+1 for this.

There's a small problem in line 63 of the patch though, number needs to be 5, and we need to include offset => $n in the arguments.

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

#4 @johnjamesjacoby
3 years ago

+1 from me also, once @flixos90's recommendations.

I'd also prefer to not use $args as a one-time variable, and do this instead:

$blogs = get_sites( array(
	'spam'       => '0',
	'deleted'    => '0',
	'archived'   => '0',
	'network_id' => get_current_network_id(),
	'fields'     => 'ids',
	'number'     => 5,
	'offset'     => $n,
	'order'      => 'DESC',
	'orderby'    => 'id',
) ); 

@spacedmonkey
3 years ago

@flixos90
3 years ago

#5 @flixos90
3 years ago

37823.5.diff has the number and offset argument correct.

#6 follow-up: @flixos90
3 years ago

  • Type changed from defect (bug) to enhancement

Maybe we should move the relevant parts here into an actual function, something like upgrade_network_sites(). This would allow us to better add unit tests for it.

#7 @jorbin
3 years ago

@jeremyfelt Can you review the above patch please

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

  • Keywords needs-unit-tests removed
  • Owner set to jeremyfelt
  • Status changed from new to reviewing

Replying to flixos90:

Maybe we should move the relevant parts here into an actual function, something like upgrade_network_sites(). This would allow us to better add unit tests for it.

37823.5.diff looks good. Let's hold off on creating an upgrade_network_sites() or similar until we explore #11869, #37799, and others a bit more. We may end up refactoring this page in other ways.

#9 follow-up: @jeremyfelt
3 years ago

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

In 38680:

Multisite: Use get_sites() in network upgrade.

Use get_sites() to generate the same query that was previously performed manually.

Props spacedmonkey, flixos90.
Fixes #37823.

#10 in reply to: ↑ 9 ; follow-up: @ocean90
3 years ago

Replying to jeremyfelt:

In 38680:

Multisite: Use get_sites() in network upgrade.

Use get_sites() to generate the same query that was previously performed manually.

Props spacedmonkey, flixos90.
Fixes #37823.

Nitpicking: spam, deleted, and archived should be integer values. 0️⃣

#11 in reply to: ↑ 10 @jeremyfelt
3 years ago

Replying to ocean90:

Nitpicking: spam, deleted, and archived should be integer values.

Good catch. :)

#12 @jeremyfelt
3 years ago

In 38819:

Multisite: Use correct types in get_sites() during network upgrade.

spam, deleted, and archived query args should be integers, not strings.

Props ocean90.
See #37823.

Note: See TracTickets for help on using tickets.