Make WordPress Core

Opened 14 years ago

Last modified 6 months ago

#17019 reviewing enhancement

add hooks for Media Library attachment counts

Reported by: kevinb's profile kevinB Owned by: sergeybiryukov's profile 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)

17019.diff (2.8 KB) - added by kevinB 14 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 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
perftest_17019.php (4.5 KB) - added by kevinB 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
perftest-results.htm (1.4 KB) - added by kevinB 14 years ago.
Results of performance test for various numbers of queriers and filters.
Perftest-Results.png (30.1 KB) - added by kevinB 14 years ago.
Results of performance test (screen capture)
17019.2.diff (3.3 KB) - added by wonderboymusic 11 years ago.
17019.3.diff (3.7 KB) - added by wonderboymusic 11 years ago.
17019.4.diff (3.1 KB) - added by wonderboymusic 11 years ago.
17019.5.diff (4.5 KB) - added by wonderboymusic 11 years ago.
170195.6.diff (3.6 KB) - added by Mte90 9 years ago.
patch refreshed for 4.6

Download all attachments as: .zip

Change History (72)

#1 @kevinB
14 years ago

  • Keywords has-patch added

#2 @kevinB
14 years ago

  • Component changed from Query to Media

#3 @scribu
14 years ago

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

#4 @scribu
14 years ago

Or maybe not so easily, but should be doable.

#5 @scribu
14 years ago

Related: #17027

#6 @kevinB
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 @scribu
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.

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

#8 @kevinB
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: @nacin
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: @kevinB
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 @nacin
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: @kevinB
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: @nacin
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: @kevinB
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?

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

#15 @scribu
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.

#16 @scribu
14 years ago

Related: #17072

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

#17 @kevinB
14 years ago

Updated patch removes $wp_query->context property.

#18 in reply to: ↑ 14 @kevinB
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.

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

@kevinB
14 years ago

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

#19 @kevinB
14 years ago

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

#20 follow-up: @scribu
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.

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

#21 follow-up: @scribu
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.

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

@kevinB
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

@kevinB
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

@kevinB
14 years ago

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

@kevinB
14 years ago

Results of performance test (screen capture)

#22 in reply to: ↑ 20 @kevinB
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.

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

#23 in reply to: ↑ 21 @kevinB
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.

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

#24 follow-up: @scribu
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 @kevinB
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 @scribu
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 @scribu
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: @scribu
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 @scribu
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 @kevinB
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?

#31 @scribu
14 years ago

Yes, please.

#32 @scribu
14 years ago

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

#33 follow-up: @wonderboymusic
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:

  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.

#34 @wonderboymusic
11 years ago

+1 = @scibu liked this

#35 @wonderboymusic
11 years ago

#17027 was marked as a duplicate.

#36 @wonderboymusic
11 years ago

#23833 was marked as a duplicate.

#37 in reply to: ↑ 33 @MikeSchinkel
11 years 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!

#38 @wonderboymusic
11 years ago

  • Keywords needs-testing added

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

#39 @helen
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 @wonderboymusic
11 years ago

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

17019.4.diff is a refresh

#41 @wonderboymusic
11 years ago

  • Keywords needs-testing removed
  • Version set to 2.5

17019.5.diff adds some unit tests

#42 @DrewAPicture
11 years ago

Related #25376

Also will need Codex once/if this goes through.

#43 @nacin
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.

#44 @nacin
11 years ago

  • Milestone changed from 3.9 to Future Release

#45 @chriscct7
9 years ago

  • Keywords needs-refresh needs-testing added

@Mte90
9 years ago

patch refreshed for 4.6

#46 @Mte90
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

#50 @Mte90
4 years ago

I just updated the patch but require testing on github

#51 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.8

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


4 years ago

#53 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


4 years ago

#55 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.8 to 5.9

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

#57 @Mte90
3 years ago

There is hope to get this merged?

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 @hellofromTonya
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.

#61 @Mte90
3 years ago

@SergeyBiryukov there are updates for this patch?

#62 @Mte90
6 months ago

@SergeyBiryukov there are updates for this patch again?

Note: See TracTickets for help on using tickets.