Opened 14 years ago
Last modified 6 months ago
#17019 reviewing enhancement
add hooks for Media Library attachment counts
Reported by: | kevinB | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 2.5 |
Component: | Query | Keywords: | has-patch needs-testing has-unit-tests early needs-testing-info |
Focuses: | Cc: |
Description
The Media Library attachments listing and edit links can be filtered via existing WP_Query and has_cap hooks. However, corresponding attachment counts are not cleanly filterable.
The submitted ticket adds the following filters:
- function wp_count_attachments() - result filter 'count_attachments'
- WP_Media_List_Table::get_views() - query filter 'attachment_orphans_query'
- function update_gallery_tab() - query filter 'gallery_attachment_count_query'
Attachments (10)
Change History (72)
#6
@
14 years ago
Agreed that making functions like this use WP_Query (with filters not suppressed) is the better and more poetic solution. I'm pleased that there's support for doing that and will submit some revised patches next week.
But what about passing in a context arg similar to my proposed query filter names? Then with every function in the app triggering the same query filters, plugins don't have to indirectly sleuth out the purpose of the query if some filtering distinction is desired. A context arg would also be very helpful in user_has_cap filter handling.
#7
@
14 years ago
The idea of a 'context' arg for WP_Query has been proposed before and I, for one, don't see anything wrong with it.
#8
@
14 years ago
- Cc kevin@… added
- Component changed from Media to Query
- Summary changed from add hooks for Media Library attachment counts to context arg for WP_Query filter application
The updated patch also adds WP_Query support for fields=count.
wp_count_attachments() filtering is removed to a different patch.
I decided to apply a context check right in apply_filters_ref_array() rather than just passing it to the filter function. That way if plugins specify a context arg in add_filter(), the increased use of get_posts() throughout wp-admin won't multiply call_user_func_array() executions.
If there is a high level of concern for retaining exact query syntax for the attachment orphan and gallery counts, my add_filter() calls in the inline code comments would be a starting point.
#9
follow-up:
↓ 12
@
14 years ago
Why not just add the filter, run WP_Query, remove the filter? There's no need for extra context values for filters/actions. It's too much overhead.
With regards to the query context value, simply keep it as a WP_Query property. It is then accessed via $this.
Please don't comment out code, just remove it -- far easier to see what's going on. Also, examples can go here in a comment, please remove those as well.
It's also not at all necessary to maintain backwards compatibility for filtering direct queries.
#10
follow-up:
↓ 11
@
14 years ago
The updated patch removes the commented-out old queries and legacy query syntax stuff. Mark's guideline about "don't change queries" was the only reason I considered that.
#11
in reply to:
↑ 10
@
14 years ago
Replying to kevinB:
The updated patch removes the commented-out old queries and legacy query syntax stuff. Mark's guideline about "don't change queries" was the only reason I considered that.
Ah, I imagine you're referring to the 3.2 plan -- that's just with regards to modifying queries to use MySQL 5.0-only constructs.
#12
in reply to:
↑ 9
;
follow-up:
↓ 13
@
14 years ago
Replying to nacin:
Why not just add the filter, run WP_Query, remove the filter? There's no need for extra context values for filters/actions. It's too much overhead.
I've attached a stripped-down patch as you suggest. I can live with that, but want to follow up on the original patch discussion just for clarity.
I'm not sure what filter you're referring to here. The whole point of this ticket is to make more wp-admin queries filterable, and scribu said to do that by running through get_posts(). That's good, but it also means every function which my plugin adds to a query-related hook will now be called multiple times via call_user_func_array() even if it only pertains to a specific case (like attachment orphan count filtering). To avoid incurring the performance hit I'll end up hooking a single wrapper filter to 'posts_where', then look at the context property to decide the actual filter to apply. Is that the direction you want WP plugin best practices to go?
#13
in reply to:
↑ 12
;
follow-up:
↓ 14
@
14 years ago
Replying to kevinB:
I'm not sure what filter you're referring to here. The whole point of this ticket is to make more wp-admin queries filterable, and scribu said to do that by running through get_posts(). That's good, but it also means every function which my plugin adds to a query-related hook will now be called multiple times via call_user_func_array() even if it only pertains to a specific case (like attachment orphan count filtering). To avoid incurring the performance hit I'll end up hooking a single wrapper filter to 'posts_where', then look at the context property to decide the actual filter to apply. Is that the direction you want WP plugin best practices to go?
Adding a context filter to all hooks is going to be way worse performance wise than adding a filter only exactly when you want it.
There are two tickets here: One to move these raw queries to WP_Query. The other is to add a context argument. This ticket should serve but one purpose.
There is some pretty good reasoning out there to add contexts to core queries, but the lack of focus of this ticket doesn't really drive that point home.
#14
in reply to:
↑ 13
;
follow-up:
↓ 18
@
14 years ago
Replying to nacin:
Adding a context filter to all hooks is going to be way worse performance wise than adding a filter only exactly when you want it.
I didn't propose adding an additional filter to all hooks. Where do you see that?
The change was to make apply_filters_ref_array(), before calling each hooked filter as it currently does, to check whether that filter was added with a context arg... and if so, to not call it if the current context doesn't match.
The changes to WP_Query and plugin.php were to apply fewer filters, not more - to let me fire my query-related filters only where I want them. So is the performance concern for the isset / empty statements? The is_array() check in add_filter? Or the optional arg to apply_filters_ref_array?
#15
@
14 years ago
17019.diff looks good to me.
However, I don't see any point in storing 'context' as a standalone property. It's already stored in $wp_query->query_vars and $wp_query->query.
#18
in reply to:
↑ 14
@
14 years ago
Replying to kevinB:
Replying to nacin:
Adding a context filter to all hooks is going to be way worse performance wise than adding a filter only exactly when you want it.
I didn't propose adding an additional filter to all hooks. Where do you see that?
I think I see where you misunderstood me. My point was that with multiple WP_Query use by core code, this:
add_filter( 'posts_where', 'my_function', 10, 2 );
causes my_function to be executed every time WP_Query applies filters for any query; I have no choice. Same for my_function2 and my_function3 which also want to filter posts_where within the same http request, but for different queries. Each one, when hooked directly to posts_where, will each be executed every N times posts_where filters are applied.
Yes, I can use the context arg to return the value unfiltered, but isn't that needless overhead which will be noted as plugin performance drag but could be avoided via more strategic filter application?
So as it stands, my plugin code will work around this by instead hooking one function to posts_where and letting it route the traffic. But most plugin / theme authors won't bother to do that, so I thought it would be better to just skip the call_user_func_array() when a context mismatch is known.
@
14 years ago
WP_Query supports query_context arg and fields=count, for attachment count filtering and other supplemental usage of get_posts()
#19
@
14 years ago
Updated both patches to avoid conflict with custom taxonomy name as described in comment:ticket:15063:19
#20
follow-up:
↓ 22
@
14 years ago
Yes, I can use the context arg to return the value unfiltered, but isn't that needless overhead which will be noted as plugin performance drag but could be avoided via more strategic filter application?
You're merely replacing the overhead caused by extra function calls with overhead caused by extra if checks.
Given that add_filter() and apply_filters() are called so many times throughout a request, I'd be very surprized if this "strategic application" would yield any speed improvements.
Anyway, first rule of optimization: show me the tests.
#21
follow-up:
↓ 23
@
14 years ago
To put it another way: you're moving the context check from the callback to the caller - apply_filters().
An if in apply_filters() is called for every filter, whereas an if in the callback is called only for that particular filter.
@
14 years ago
Optional context arg limits application of WP_Query filters: add_filter( array('tagname', 'context'), 'myfunc' ); Core get_posts() calls pass query_context in $args
@
14 years ago
Evaluate execution time for superfluous call_user_func_array() calls by current apply_filters_ref_array(), and execution time of additional empty statement to avoid those calls
#22
in reply to:
↑ 20
@
14 years ago
Replying to scribu:
You're merely replacing the overhead caused by extra function calls with overhead caused by extra if checks.
Given that add_filter() and apply_filters() are called so many times throughout a request, I'd be very surprized if this "strategic application" would yield any speed improvements.
Yes, the is_array() check in add_filter() was a mistake so I've removed that in the revised patch. apply_filters() is not affected, as the context check is in apply_filters_ref_array() - used mainly by WP_Query to the tune of 26 per typical query by my count.
Assuming a new optional arg to add_filter would not be appreciated either, I'm instead proposing a new function:
function add_context_filter($tag, $context, $function_to_add, $priority = 10, $accepted_args = 1)
Functionally equivalent alternatives welcome.
Anyway, first rule of optimization: show me the tests.
Test script and results are attached. Now you might argue that the improvement for likely use cases is insignificant, but I don't see how you can reject this for performance concerns.
#23
in reply to:
↑ 21
@
14 years ago
Replying to scribu:
To put it another way: you're moving the context check from the callback to the caller - apply_filters().
Yes, but the caller is apply_filters_ref_array().
And moving the context check to that caller also reduces the risk of query destruction by filters which incorrectly return null. If the offending filter was added for a specific context, it will only wreck one query and not all of them.
#24
follow-up:
↓ 25
@
14 years ago
apply_filters() is not affected, as the context check is in apply_filters_ref_array() - used mainly by WP_Query to the tune of 26 per typical query by my count.
Not for long: #16661
#25
in reply to:
↑ 24
@
14 years ago
Replying to scribu:
apply_filters() is not affected, as the context check is in apply_filters_ref_array() - used mainly by WP_Query to the tune of 26 per typical query by my count.
Not for long: #16661
Then how about instead making WP_Query call a new function apply_query_filters() to support a context check and whatever other query-specific filtering issues might emerge?
#26
@
14 years ago
That might be an idea.
Correctly handling WP_Query hooks has got to be the most error-prone task in WP development that I know of, precisely because they can affect every single instance.
Note that this would have to extend to all hooks: 'parse_query', 'pre_get_posts', 'the_posts' etc.
Important consideration: correctly handling WP_Queries and/or hooks without a context.
#27
@
14 years ago
Actually, no, manually assigned contexts wouldn't solve the problem I'm thinking about:
The same hook runs both on '/', as well as on '/tag/a-tag', as well as on '/a-page' etc.
To filter those queries, you would need a dynamically generated context, which is what we already have, in the form of conditional tags.
#28
follow-up:
↓ 30
@
14 years ago
So, I don't think apply_query_filters() is worth the effort, just for those few get_posts() calls throughout the admin.
#29
@
14 years ago
If you want to squeeze out a little extra performance, just check is_admin() before adding the filters.
#30
in reply to:
↑ 28
@
14 years ago
Replying to scribu:
So, I don't think apply_query_filters() is worth the effort, just for those few get_posts() calls throughout the admin.
If that's the final analysis can we shift focus back to getting 17019.diff approved for manually-assigned 'query_context' in $wp_query->query_variables?
#32
@
14 years ago
- Summary changed from context arg for WP_Query filter application to add hooks for Media Library attachment counts
#33
follow-up:
↓ 37
@
11 years ago
- Milestone changed from Awaiting Review to 3.7
Tidy'd up this patch, it was blowing up against trunk.
The patch does the following:
- Allows you to pass
query_context
toWP_Query
- Adds
count
as an allowed value forfields
- Cleans up a few unfilterable direct SQL calls in media admin by using
get_posts()
withquery_context
passed to it
I like it.
#37
in reply to:
↑ 33
@
11 years ago
Replying to wonderboymusic:
Tidy'd up this patch, it was blowing up against trunk.
The patch does the following:
- Allows you to pass
query_context
toWP_Query
- Adds
count
as an allowed value forfields
- Cleans up a few unfilterable direct SQL calls in media admin by using
get_posts()
withquery_context
passed to itI like it.
Sweet!
#38
@
11 years ago
- Keywords needs-testing added
Refreshed against trunk - after testing, this could maybe go in
#39
@
11 years ago
- Milestone changed from 3.7 to Awaiting Review
Works as advertised on the surface, unfortunately late to be adding query_context and count without further testing and feedback.
#40
@
11 years ago
- Keywords needs-unit-tests added
- Milestone changed from Awaiting Review to 3.9
17019.4.diff is a refresh
#41
@
11 years ago
- Keywords needs-testing removed
- Version set to 2.5
17019.5.diff adds some unit tests
#43
@
11 years ago
Adding a "count" flag to WP_Query is a good step, but let's avoid doing query_context in this ticket. That should be separate. Really, both of these should be distinct, Query-specific tickets, as they're new API, versus being buried in a "add hooks for Media Library attachment counts" ticket.
This needs database considerations in terms of COUNT(*) versus COUNT(1) versus COUNT(ID). pento probably knows which is best.
post_count is normally an integer so we should cast it here as well, rather than after. I'd also use new WP_Query( $args )
followed by $attachments = $query->post_count
so the return value is obvious.
#46
@
9 years ago
- Keywords needs-unit-tests needs-refresh removed
I updated the patch respect to the last the first part that exist anymore because on get_view
there is no a query sql.
This ticket was mentioned in Slack in #core by mte90. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by mte90. View the logs.
7 years ago
This ticket was mentioned in PR #1052 on WordPress/wordpress-develop by Mte90.
4 years ago
#49
- Keywords has-unit-tests added
Trac ticket: https://core.trac.wordpress.org/ticket/17019
I just refreshed the code but didn't tested
This ticket was mentioned in Slack in #core by mte90. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by mte90. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-test by boniu91. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
3 years ago
#60
@
3 years ago
- Keywords early needs-testing-info added
- Milestone changed from 5.9 to Future Release
With 5.9 feature freeze happening in 7 days, it's too late in the release cycle for this ticket as it touches query. It will need more eyes on it and lots of testing early in a release cycle. Moving it to Future Release
and marking as early
.
As once it's ready for testers to do manual testing and provide test reports, please add step-by-step instructions on what is needed to do these tests including any scripts, dependencies, or setups. Marking as needs-testing-info
.
For component maintainers, please move this into a milestone when it can be done early in the release cycle.
-1 to 'gallery_attachment_count_query'. Can easily be made to use get_posts().