WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 5 weeks ago

Last modified 7 days ago

#47599 closed defect (bug) (fixed)

Return for short circuits for multisite classes.

Reported by: spacedmonkey Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2
Component: Networks and Sites Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: multisite Cc:
PR Number:

Description (last modified by SergeyBiryukov)

In [44983] we introduced pre query filters in multisite classes. However this short circuit acts differently from other short circuits and still continues to execute. After the filter runs, it should exit straight away.

Attachments (9)

47599.diff (4.9 KB) - added by spacedmonkey 4 months ago.
from_ticket_47599_by_adamsilverstein__Pull_Request_76__WordPresswordpress-develop_2019-08-02_11-24-21.jpg (520.3 KB) - added by adamsilverstein 3 months ago.
47599.2.diff (6.5 KB) - added by adamsilverstein 2 months ago.
47599.3.diff (7.8 KB) - added by adamsilverstein 2 months ago.
47599.4.diff (9.3 KB) - added by adamsilverstein 2 months ago.
47599.5.diff (9.4 KB) - added by adamsilverstein 5 weeks ago.
47599.6.diff (9.5 KB) - added by adamsilverstein 5 weeks ago.
47599.7.diff (9.5 KB) - added by SergeyBiryukov 5 weeks ago.
47599.8.diff (10.2 KB) - added by adamsilverstein 5 weeks ago.

Download all attachments as: .zip

Change History (25)

@spacedmonkey
4 months ago

#1 @SergeyBiryukov
4 months ago

  • Description modified (diff)

#2 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.3

#3 @adamsilverstein
3 months ago

@spacedmonkey Thanks for the patch!

Can you explain what problem this patch solves? are you still seeing database queries after returning an array of ids from this filter?

noticing that after this patch we loose handling in https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-network-query.php#L247-L290 - including args like fields, count and update_network_cache.

Also the function get_networks usually returns an array of WP_Network objects - after this change it looks more like you the function would return an array of IDs, is that right?

#4 @adamsilverstein
3 months ago

Reviewing this diff without whitespace changes I can see the change here is smaller than what i thought and does exactly what our other filters do.

This looks good, thanks @spacedmonkey!

https://github.com/WordPress/wordpress-develop/pull/76/files?w=1

#5 @adamsilverstein
3 months ago

  • Keywords needs-unit-tests added

This could use a unit test to verify that these functions return early when the filters return non null values.

#6 @adamsilverstein
2 months ago

Question from @felipeelia related - https://core.trac.wordpress.org/ticket/45800#comment:21

Should we really be setting $this->sites here? before the filter depending on the query this could be set to ids, full objects or nothing (for a count request). Since we pass a reference to $this in the filter, maybe we should leave it up to filter consumers to set $this->sites?

In 47599.2.diff I updated our existing unit tests to account for requesting full objects not just ids. in this case previously additional database requests were made and the tests fail. after this patch the tests pass - no additional queries are run when a non null value is returned from the filter.

#7 @adamsilverstein
2 months ago

In 47599.3.diff:

  • improve documentation of filter, return type
  • don't set this->sites with return value

Diff without whitespace: https://github.com/WordPress/wordpress-develop/pull/78/files?w=1

#8 @adamsilverstein
2 months ago

47599.4.diff improve filter docs.

#9 @adamsilverstein
8 weeks ago

  • Keywords 2nd-opinion has-unit-tests added; needs-unit-tests removed

@spacedmonkey or @felipeelia - appreciate a review of 47599.4.diff when you have a chance!

#10 @felipeelia
8 weeks ago

Seems good. The only consideration I have is that $network_data type should follow $site_data and be array|int|null as well :)

#11 follow-up: @adamsilverstein
5 weeks ago

  • Keywords commit added; 2nd-opinion removed

47599.6.diff includes some doc block cleanup after reviewing https://core.trac.wordpress.org/changeset/46087

#12 in reply to: ↑ 11 @SergeyBiryukov
5 weeks ago

Replying to adamsilverstein:

47599.6.diff includes some doc block cleanup after reviewing https://core.trac.wordpress.org/changeset/46087

47599.7.diff adjusts the alignment in lines 215-217 in class-wp-network-query.php to account for the added int, looks good to me otherwise :)

#13 @adamsilverstein
5 weeks ago

47599.8.diff ensure tess run against returned results so they pass.

#14 @adamsilverstein
5 weeks ago

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

In 46100:

Multisite: improve sites_pre_query and networks_pre_query filters, avoiding db queries.

Improve the pre_query filters in multisite classes introduced in r44983. Return (non null) values immediately,
avoiding the database queries entirely, similar to other pre_query filters.

Props spacedmonkey, SergeyBiryukov, felipeelia.
Fixes #47599.

#15 @adamsilverstein
5 weeks ago

  • Keywords needs-dev-note added

Added needs-dev-note, we should document this change along with the new comments pre filter.

This ticket was mentioned in Slack in #core by justinahinon. View the logs.


7 days ago

Note: See TracTickets for help on using tickets.