#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 has-dev-note |
Focuses: | multisite | Cc: |
Description (last modified by )
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)
Change History (28)
#4
@
5 years 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
@
5 years 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
@
5 years 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
@
5 years 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
@
5 years ago
47599.4.diff improve filter docs.
#9
@
5 years 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
@
5 years 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:
↓ 12
@
5 years 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
@
5 years 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
@
5 years ago
47599.8.diff ensure tess run against returned results so they pass.
#15
@
5 years ago
- Keywords needs-dev-note added
Added needs-dev-note
, we should document this change along with the new comments pre filter.
@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 ofWP_Network
objects - after this change it looks more like you the function would return an array of IDs, is that right?