WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 months ago

Last modified 6 weeks ago

#45749 closed enhancement (fixed)

Add short circuits for multisite classes.

Reported by: spacedmonkey Owned by: adamsilverstein
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.6
Component: Networks and Sites Keywords: has-patch has-dev-note
Focuses: multisite Cc:

Description

The multisite query classes, should have the ability to short circuit results, similar to posts_pre_query to WP_Query (#36687) and posts_pre_query in WP_User_Query (#44169). This short circuit let you load sites / network from another source like ElasticPress.

Attachments (13)

45749.diff (3.6 KB) - added by spacedmonkey 6 months ago.
45749.1.diff (2.8 KB) - added by spacedmonkey 6 months ago.
45749.2.diff (2.9 KB) - added by adamsilverstein 3 months ago.
Image 2019-03-13 at 2.48.39 PM.png (149.5 KB) - added by adamsilverstein 3 months ago.
45749.3.diff (4.1 KB) - added by adamsilverstein 3 months ago.
Image 2019-03-13 at 5.28.08 PM.png (27.1 KB) - added by adamsilverstein 3 months ago.
Image 2019-03-13 at 5.33.12 PM.png (144.3 KB) - added by adamsilverstein 3 months ago.
45749.4.diff (4.6 KB) - added by adamsilverstein 3 months ago.
45749.5.diff (5.6 KB) - added by spacedmonkey 3 months ago.
45749.6.diff (7.4 KB) - added by adamsilverstein 3 months ago.
45749.7.diff (12.5 KB) - added by spacedmonkey 3 months ago.
45749.8.diff (17.5 KB) - added by adamsilverstein 3 months ago.
45749.9.diff (8.6 KB) - added by adamsilverstein 3 months ago.

Download all attachments as: .zip

Change History (42)

@spacedmonkey
6 months ago

#1 @spacedmonkey
6 months ago

There are two way I have gone about this.

  1. In 45749.diff there are no new filters added. This short circult, would work by using the existing action pre_get_sites which would allow you to change the class property of site_ids.
  2. In 45749.1.diff adds a new filter sites_pre_query similar to other filters existing in core.

#2 @spacedmonkey
5 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#3 @desrosj
5 months ago

  • Milestone changed from Awaiting Review to Future Release

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


5 months ago

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


4 months ago

#6 @adamsilverstein
4 months ago

  • Milestone changed from Future Release to 5.2
  • Owner changed from spacedmonkey to adamsilverstein

#7 @adamsilverstein
3 months ago

Diff without whitespace changes: https://cl.ly/39d47b7b732f

#8 @adamsilverstein
3 months ago

45749.3.diff add a test for sites_pre_query

#9 @adamsilverstein
3 months ago

Note: tests are failing due to phpcs after this patch.

Fixing these issues would require changing queries so leaving as is for now.

https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/506014981

#10 @desrosj
3 months ago

@adamsilverstein how do you feel about this for 5.2?

#11 @adamsilverstein
3 months ago

@desrosj feeling good about the patch as is and will commit today, I'm going to leave the phpcs errors in existing code untouched to avoid any possibility of breaking changes.

#12 @adamsilverstein
3 months ago

  • Keywords commit added

Working on this now, verifying tests pass.

#13 @spacedmonkey
3 months ago

@adamsilverstein This ticket is for both wp_site_query and wp_network_query filters. Is it possible to add the network query in the same patch or should this be a different ticket?

#14 @adamsilverstein
3 months ago

@spacedmonkey - We can add it in the same patch - is this the code in https://core.trac.wordpress.org/attachment/ticket/45749/45749.diff ? I missed that separate code earlier, usually diffs are cumulative :)

#15 @adamsilverstein
3 months ago

Note: I'm seeing test failures after this patch: https://cl.ly/75742cc9d73a https://travis-ci.org/adamsilverstein/wordpress-develop-fork/jobs/509641958

Tests_User_Query::test_sites_pre_query_filter_should_bypass_database_query - Error: Class 'WP_Site_Query' not found at tests/phpunit/tests/user/query.php:1737

#16 @spacedmonkey
3 months ago

Updated the patch, mixing two patches together and moving the test to the correct file.
@adamsilverstein .

https://core.trac.wordpress.org/attachment/ticket/45749/45749.5.diff

#17 @spacedmonkey
3 months ago

In 45749.6.diff is a different patch, for network query. Both 45749.5.diff and 45749.6.diff should be committed as different commits.

#18 @adamsilverstein
3 months ago

@spacedmonkey Thanks! Running tests on 45749.5.diff, code looks good.

Does this look good for the commit message?

Multisite: add a sites_pre_query filter to short circuit WP_Site_Query queries.
Add a new filter sites_pre_query - which returns null by default. Return a non-null value to bypass WordPress's default get_sites queries. Similar to the posts_pre_query filter for WP_Query added in #36687. This filter lets you short circuit the WP_Site_Query query to return your own results.
Developers should note that filtering functions that require pagination information are encouraged to set the found_sites property of the WP_Site_Query object, passed to the filter by reference. If WP_Site_Query does not perform a database query, it will not have enough information to generate these values itself.

#19 @adamsilverstein
3 months ago

@spacedmonkey I can't seem to find 45749.6.diff - can you re-upload?

#20 @adamsilverstein
3 months ago

45749.5.diff is 45749.5.diff plus autofixes from phpcbf (applied with composer run format) to fix some style errors.

Seeing some test failures: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/509713899

Last edited 3 months ago by adamsilverstein (previous) (diff)

#21 @spacedmonkey
3 months ago

Reuploaded as 45749.7.diff.

Commmit message looks good to me.

#22 @adamsilverstein
3 months ago

Great! thanks for the new 45749.7.diff - these are so similar I missed that they are different at first.

Since they are so closely related, we can probably commit them together and I will expand my commit message to cover networks_pre_query.

I am still seeing test failures in Travis for .5 https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/509713899 and .7 https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/509934427

#23 @adamsilverstein
3 months ago

45749.8.diff combines both patches and fixes a few things that broke tests, some are still failing. @spacedmonkey is working on a fix.

#24 @adamsilverstein
3 months ago

45749.9.diff - trying to simplify this a bit:

  • uses local vs class variables for $network_ids and $site_ids
  • removes unrelated whitespace changes except those fixable by phpcbf

viewing the diff with whitespace changes disabled the changes are now very small - https://cl.ly/e61374e40f16 (or view here and turn off whitespace changes under diff settings: https://github.com/WordPress/wordpress-develop/pull/49/files)

Last edited 3 months ago by adamsilverstein (previous) (diff)

#26 @adamsilverstein
3 months ago

  • Keywords needs-dev-note added

#27 @adamsilverstein
3 months ago

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

In 44983:

Multisite: add new sites_pre_query and networks_pre_query filters to short circuit WP_Site_Query and WP_Network_Query queries.

Similar to the posts_pre_query filter for WP_Query added in #36687. These filters lets you short circuit the queries to return your own results.

Add a new filter sites_pre_query - which returns null by default. Return a non-null value to bypass WordPress's default get_sites queries.

Developers should note that filtering functions that require pagination information are encouraged to set the found_sites property of the WP_Site_Query object, passed to the filter by reference. If WP_Site_Query does not perform a database query, it will not have enough information to generate these values itself.

Add a new filter networks_pre_query - which returns null by default. Return a non-null value to bypass WordPress's default get_networks queries.

Developers should note that filtering functions that require pagination information are encouraged to set the found_networks property of the WP_Network_Query object, passed to the filter by reference. If WP_Network_Query does not perform a database query, it will not have enough information to generate these values itself.

Props spacedmonkey.
Fixes #45749.

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


3 months ago

#29 @desrosj
6 weeks ago

  • Keywords has-dev-note added; commit needs-dev-note removed
Note: See TracTickets for help on using tickets.