WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 weeks ago

#17019 new enhancement

add hooks for Media Library attachment counts

Reported by: kevinB Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.5
Component: Query Keywords: has-patch needs-unit-tests
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 (9)

17019.diff (2.8 KB) - added by kevinB 3 years ago.
WP_Query supports query_context arg and fields=count, for attachment count filtering and other supplemental usage of get_posts()
media-library-attachment-filters_3.2.patch (12.0 KB) - added by kevinB 3 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
perftest_17019.php (4.5 KB) - added by kevinB 3 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
perftest-results.htm (1.4 KB) - added by kevinB 3 years ago.
Results of performance test for various numbers of queriers and filters.
Perftest-Results.png (30.1 KB) - added by kevinB 3 years ago.
Results of performance test (screen capture)
17019.2.diff (3.3 KB) - added by wonderboymusic 9 months ago.
17019.3.diff (3.7 KB) - added by wonderboymusic 7 months ago.
17019.4.diff (3.1 KB) - added by wonderboymusic 3 months ago.
17019.5.diff (4.5 KB) - added by wonderboymusic 3 months ago.

Download all attachments as: .zip

Change History (53)

comment:1 kevinB3 years ago

  • Keywords has-patch added

comment:2 kevinB3 years ago

  • Component changed from Query to Media

comment:3 scribu3 years ago

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

comment:4 scribu3 years ago

Or maybe not so easily, but should be doable.

comment:5 scribu3 years ago

Related: #17027

comment:6 kevinB3 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.

comment:7 scribu3 years ago

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

Version 0, edited 3 years ago by scribu (next)

comment:8 kevinB3 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.

comment:9 follow-up: nacin3 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.

comment:10 follow-up: kevinB3 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.

comment:11 in reply to: ↑ 10 nacin3 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.

comment:12 in reply to: ↑ 9 ; follow-up: kevinB3 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?

comment:13 in reply to: ↑ 12 ; follow-up: nacin3 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.

comment:14 in reply to: ↑ 13 ; follow-up: kevinB3 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?

Last edited 3 years ago by kevinB (previous) (diff)

comment:15 scribu3 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.

comment:16 scribu3 years ago

Related: #17072

Last edited 3 years ago by scribu (previous) (diff)

comment:17 kevinB3 years ago

Updated patch removes $wp_query->context property.

comment:18 in reply to: ↑ 14 kevinB3 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.

Last edited 3 years ago by kevinB (previous) (diff)

kevinB3 years ago

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

comment:19 kevinB3 years ago

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

comment:20 follow-up: scribu3 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.

Last edited 3 years ago by scribu (previous) (diff)

comment:21 follow-up: scribu3 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.

Last edited 3 years ago by scribu (previous) (diff)

kevinB3 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

kevinB3 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

kevinB3 years ago

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

kevinB3 years ago

Results of performance test (screen capture)

comment:22 in reply to: ↑ 20 kevinB3 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.

Last edited 3 years ago by kevinB (previous) (diff)

comment:23 in reply to: ↑ 21 kevinB3 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.

Last edited 3 years ago by kevinB (previous) (diff)

comment:24 follow-up: scribu3 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

comment:25 in reply to: ↑ 24 kevinB3 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?

comment:26 scribu3 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.

comment:27 scribu3 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.

comment:28 follow-up: scribu3 years ago

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

comment:29 scribu3 years ago

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

comment:30 in reply to: ↑ 28 kevinB3 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?

comment:31 scribu3 years ago

Yes, please.

comment:32 scribu3 years ago

  • Summary changed from context arg for WP_Query filter application to add hooks for Media Library attachment counts

wonderboymusic9 months ago

comment:33 follow-up: wonderboymusic9 months 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:

  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.

comment:34 wonderboymusic9 months ago

+1 = @scibu liked this

comment:35 wonderboymusic9 months ago

#17027 was marked as a duplicate.

comment:36 wonderboymusic9 months ago

#23833 was marked as a duplicate.

comment:37 in reply to: ↑ 33 MikeSchinkel9 months ago

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!

wonderboymusic7 months ago

comment:38 wonderboymusic7 months ago

  • Keywords needs-testing added

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

comment:39 helen7 months 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.

wonderboymusic3 months ago

comment:40 wonderboymusic3 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

17019.4.diff is a refresh

wonderboymusic3 months ago

comment:41 wonderboymusic3 months ago

  • Keywords needs-testing removed
  • Version set to 2.5

17019.5.diff adds some unit tests

comment:42 DrewAPicture3 months ago

Related #25376

Also will need Codex once/if this goes through.

comment:43 nacin7 weeks 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.

comment:44 nacin7 weeks ago

  • Milestone changed from 3.9 to Future Release
Note: See TracTickets for help on using tickets.