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

[iandunn]

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.

[iandunn]

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?

[iandunn]

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]

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?

[iandunn]

Sounds good to me :)

[jeremyfelt]

@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. :)

[flixos90]

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.