WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 7 months ago

#37217 closed enhancement (fixed)

get_network_by_path should use WP_Network_Query

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

Description

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 16 months ago.
37217.2.diff (2.6 KB) - added by flixos90 13 months ago.
37217.3.diff (5.2 KB) - added by flixos90 10 months ago.
37217.4.diff (8.1 KB) - added by jeremyfelt 10 months ago.
37217.5.diff (2.1 KB) - added by jeremyfelt 7 months ago.

Download all attachments as: .zip

Change History (18)

@spacedmonkey
16 months ago

#1 @spacedmonkey
15 months ago

  • Keywords has-patch 4.7-early added

#2 @flixos90
14 months 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
14 months 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
14 months ago

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

#5 @jorbin
13 months 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.


13 months ago

#7 @flixos90
13 months 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.

@flixos90
13 months ago

#8 @flixos90
13 months 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?

@flixos90
10 months ago

#9 @flixos90
10 months 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. :)

@jeremyfelt
10 months ago

#10 @jeremyfelt
10 months 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
8 months 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.

@jeremyfelt
7 months ago

#12 @jeremyfelt
7 months 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 months 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.