WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 4 weeks ago

#39879 closed defect (bug) (fixed)

wp_get_sites() with a false limit no longer returns all sites

Reported by: iandunn Owned by: flixos90
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.6
Component: Networks and Sites Keywords: has-patch has-unit-tests commit
Focuses: multisite Cc:

Description

It looks like r37653 (from #36994) broke back-compat with calling wp_get_sites( array( 'limit' => false ) ), because it only defines $args['number'] when $args['limit'] contains a numeric value, which boolean false is not.

Because of this, any calls to wp_get_sites( array( 'limit' => false ) ) will now only return the default 100 sites, rather than all of them.

Attachments (2)

39879.1.diff (2.8 KB) - added by iandunn 2 months ago.
39879.diff (462 bytes) - added by flixos90 6 weeks ago.

Download all attachments as: .zip

Change History (10)

@iandunn
2 months ago

#1 @iandunn
2 months ago

  • Keywords has-patch has-unit-tests added

39879.1.diff is a first pass at a test and fix. I haven't had time to test it thoroughly, though, so I'd appreciate more eyes on it.

A new test wasn't actually required here; the existing tests would have caught this bug if a sufficient number of sites existed in the database. Creating that many sites introduces a performance issue to the test suite, though. The time required to run the wp-get-site group increases from ~10 seconds to ~50.

I don't have a good solution to that right now; does anyone have any ideas?

#2 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 4.8

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


8 weeks ago

#4 @iandunn
7 weeks ago

Creating that many sites introduces a performance issue to the test suite

Maybe one way to solve that would be to create a flag to control whether or not the extra sites would be created. It would default to false, so developers could continue running the group with a small amount of data during development.

Then, before creating a patch, they could set the flag to true with something like USE_LARGE_DATA=1 phpunit --group=wp-get-site to run the full group. Travis could be configured to always set the flag to true.

@flixos90
6 weeks ago

#5 @flixos90
6 weeks ago

  • Keywords 2nd-opinion added

Thanks for the report and patch @iandunn. I think it is easier to not use PHP_INT_MAX, but rather an empty value so that the LIMIT clause is ignored. 39879.diff uses that approach.

Regarding the tests, I'm not sure whether we should thorougly test this. While I generally would be a 100% for testing, as you already mentioned there are performance issues when creating 100 sites. We could maybe write a test for WP_Site_Query though to check whether passing an empty value to number also sets the query argument to an empty value, although this would be a "very nitpicking" test. It would be better if we could check the query vars from the WP_Site_Query instance that wp_get_sites() instantiates through get_sites(), but since that one is incapsulated, it wouldn't be possible.

Ping @jeremyfelt, do you think tests make sense here regarding the circumstances, or can we move forward without?

#6 @iandunn
6 weeks ago

Sounds good to me :)

#7 @jeremyfelt
5 weeks ago

  • Keywords commit added; 2nd-opinion removed
  • Owner set to flixos90
  • Status changed from new to assigned

@flixos90 I think we're okay with 39879.diff and no additional tests. If we had a lighter way of mocking the data, tests would be nice, but I don't think it's worth the effort at this point. :)

#8 @flixos90
4 weeks ago

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

In 40372:

Multisite: Fix wp_get_sites() to return an unlimited amount of sites when passing a falsy limit argument.

Props iandunn for the original patch.
Fixes #39879. See #35791.

Note: See TracTickets for help on using tickets.