Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#37217 closed enhancement (fixed)

get_network_by_path should use WP_Network_Query

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


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.

Attachments (5)

37217.diff (10.4 KB) - added by spacedmonkey 8 years ago.
37217.2.diff (2.6 KB) - added by flixos90 8 years ago.
37217.3.diff (5.2 KB) - added by flixos90 7 years ago.
37217.4.diff (8.1 KB) - added by jeremyfelt 7 years ago.
37217.5.diff (2.1 KB) - added by jeremyfelt 7 years ago.

Download all attachments as: .zip

Change History (18)

8 years ago

#1 @spacedmonkey
8 years ago

  • Keywords has-patch 4.7-early added

#2 @flixos90
8 years ago

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.

#3 @jeremyfelt
8 years ago

  • Keywords 4.7-early removed
  • Milestone changed from Awaiting Review to 4.7

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.

#4 @jeremyfelt
8 years ago

  • Owner set to jeremyfelt
  • Status changed from new to reviewing

#5 @jorbin
8 years ago

  • Keywords needs-unit-tests added

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

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

8 years ago

#7 @flixos90
8 years ago

  • Keywords needs-refresh added
  • Milestone changed from 4.7 to Future Release
  • Owner changed from jeremyfelt to 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.

8 years ago

#8 @flixos90
8 years ago

  • Keywords needs-refresh removed

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?

7 years ago

#9 @flixos90
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

7 years ago

#10 @jeremyfelt
7 years ago

  • Milestone changed from Future Release to 4.8

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.

#11 @flixos90
7 years ago

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

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.

7 years ago

#12 @jeremyfelt
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#13 @jeremyfelt
7 years ago

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

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.

Note: See TracTickets for help on using tickets.