get_network_by_path should use WP_Network_Query

[spacedmonkey]

The get_network_by_path is used in the bootstrap process to load the network object. With the new WP_Network_Query class, we should use that to load the network object. Using WP_Network_Query we gain filters and caching.

[flixos90]

I'm not sure that we should deprecate WP_Network::get_by_path(). Is there a particular reason for the deprecation? I think the code you have in get_network_by_path() could as well be in WP_Network::get_by_path(), and then get_network_by_path() would only be a wrapper. In most cases in WordPress functions are wrappers for class methods, not the other way around afaik. Whichever way we do it, I don't think we need to deprecate anything here.

[jeremyfelt]

I'm going to remove the 4.7-early tag on this and assign to the 4.7 release for continued discussion. I don't think we should rush to use WP_Network_Query here until we're confident that tests support the change.

[jorbin]

This looks like it should have some unit tests to ensure there is no breakage.

[flixos90]

This will need a refreshed patch. Let's punt it for now, but leave open a possibility that we can add it back to the milestone if we get to a good update anytime soon enough.

[flixos90]

37217.2.diff​ is a simplified patch that only replaces the database calls in WP_Network::get_by_path() by get_networks() calls.

@jeremyfelt What do you have in mind for unit tests? There is Tests_Multisite_Bootstrap::test_get_network_by_path(), and I made sure these tests pass. Do we need further ones?

[flixos90]

In 37217.3.diff​ I added an additional unit test with data provider for when using get_network_by_path() with the $using_paths flag set to false. I made sure the both tests pass with the existing as well as the new code using WP_Network_Query.
The new test actually uncovered an issue with the previous patch where I accidentally had ASC in line 365 of the WP_Network class. :)

[jeremyfelt]

The changes to WP_Network::get_by_path look good, I think we can move forward with this.

I made a few changes in 37217.4.diff​:

• Only run the no path tests when an external object cache is available. Otherwise I was getting a bunch of test errors locally with the tests in pluggable.php. I think we're safe to only focus on object cache setups here as that is the only time this code will fire.
• Remove an old @todo note that doesn't really make sense any more.
• Add test cases for how we deal with the lookup for a network with multiple paths.

[flixos90]

In 40102:

Multisite: Use WP_Network_Query in WP_Network::get_by_path().

An additional unit test has been introduced to verify the method works properly when using an external object cache.

Props spacedmonkey, jeremyfelt.
Fixes #37217.

[jeremyfelt]

In [40102], we introduced a test and data provider that asserts a series of conditions that would fit a configuration in which path segments are limited to 0. However, instead of filtering the path segments, we force $using_paths to 0, which doesn't limit the search to single path segments, but ignores paths entirely when doing the database lookup. The results from this query can be unpredictable with the assigned data provider. This was causing an issue in my local environment when running tests with an object cache enabled.

37217.5.diff​ adjusts the test to filter the network path segments count to 0, which makes these tests more predictable. This also allows us to always run the test rather than only in an object cache configuration.

[jeremyfelt]

In 40342:

Tests: Clarify zero path segment tests for get_network_by_path().

The set of assertions in this data provider intend to test a 0 path segment configuration rather than the use of paths.

Fixes #37217.