add hooks for Media Library attachment counts

[kevinB]

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'

[scribu]

-1 to 'gallery_attachment_count_query'. Can easily be made to use get_posts().

[scribu]

Or maybe not so easily, but should be doable.

[scribu]

Related: #17027

[kevinB]

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.

[scribu]

The idea of a 'context' arg for WP_Query has been proposed before and I, for one, don't see anything wrong with it.

[kevinB]

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.

[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.

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.

[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.

[nacin]

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.

[kevinB]

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?

[nacin]

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.

[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?

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?

[scribu]

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.

[scribu]

Related: #17072

[kevinB]

Updated patch removes $wp_query->context property.

[kevinB]

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.

[kevinB]

WP_Query supports query_context arg and fields=count, for attachment count filtering and other supplemental usage of get_posts()

[kevinB]

Updated both patches to avoid conflict with custom taxonomy name as described in comment:ticket:15063:19

[scribu]

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.

[scribu]

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.

[kevinB]

Optional context arg limits application of WP_Query filters: add_filter( array('tagname', 'context'), 'myfunc' ); Core get_posts() calls pass query_context in $args

[kevinB]

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

[kevinB]

Results of performance test for various numbers of queriers and filters.

[kevinB]

Results of performance test (screen capture)

[kevinB]

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.

[kevinB]

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.

[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

[kevinB]

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?

[scribu]

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.

[scribu]

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.

[scribu]

So, I don't think apply_query_filters() is worth the effort, just for those few get_posts() calls throughout the admin.

[scribu]

If you want to squeeze out a little extra performance, just check is_admin() before adding the filters.

[kevinB]

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?

[scribu]

Yes, please.

[wonderboymusic]

Tidy'd up this patch, it was blowing up against trunk.

The patch does the following:

1. Allows you to pass query_context to WP_Query
2. Adds count as an allowed value for fields
3. Cleans up a few unfilterable direct SQL calls in media admin by using get_posts() with query_context passed to it

I like it.

[wonderboymusic]

+1 = @scibu liked this

[wonderboymusic]

#17027 was marked as a duplicate.

[wonderboymusic]

#23833 was marked as a duplicate.

[MikeSchinkel]

Replying to wonderboymusic:

Tidy'd up this patch, it was blowing up against trunk.

The patch does the following:

1. Allows you to pass query_context to WP_Query
2. Adds count as an allowed value for fields
3. Cleans up a few unfilterable direct SQL calls in media admin by using get_posts() with query_context passed to it

I like it.

Sweet!

[wonderboymusic]

Refreshed against trunk - after testing, this could maybe go in

[helen]

Works as advertised on the surface, unfortunately late to be adding query_context and count without further testing and feedback.

[wonderboymusic]

17019.4.diff​ is a refresh

[wonderboymusic]

17019.5.diff​ adds some unit tests

[DrewAPicture]

Related #25376

Also will need Codex once/if this goes through.

[nacin]

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.

[Mte90]

patch refreshed for 4.6

[Mte90]

I updated the patch respect to the last the first part that exist anymore because on get_view there is no a query sql.

Trac ticket: https://core.trac.wordpress.org/ticket/17019

I just refreshed the code but didn't tested

[Mte90]

I just updated the patch but require testing on github

[Mte90]

There is hope to get this merged?

[hellofromTonya]

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.

[Mte90]

@SergeyBiryukov there are updates for this patch?

[Mte90]

@SergeyBiryukov there are updates for this patch again?