WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 4 years ago

#17026 closed enhancement (maybelater)

parent_dropdown() is not filterable

Reported by: kevinB Owned by: boonebgorges
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:
PR Number:

Description

The parent_dropdown() function is no longer used by the WP core, but is not deprecated and remains in use by some plugins. Since other plugins may modify page editing access via existing WP_Query hooks, the query executed by parent_dropdown() also need to be filterable.

The corresponding patch adds query filter 'parent_dropdown_query'

Attachments (3)

parent-dropdown_filters_3.2.patch (812 bytes) - added by kevinB 9 years ago.
query filter for parent_dropdown()
17026.diff (773 bytes) - added by wonderboymusic 6 years ago.
17026.2.diff (996 bytes) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (17)

@kevinB
9 years ago

query filter for parent_dropdown()

#1 follow-up: @scribu
9 years ago

  • Keywords close added

-1. We should instead make parent_dropdown() use get_posts() instead.

This would require that the 'fields' query var (#14777) accept an array and not just 'all' or 'ids'.

#2 @scribu
9 years ago

  • Summary changed from add hook for parent_dropdown() to parent_dropdown() is not filterable

#3 in reply to: ↑ 1 @kevinB
9 years ago

  • Cc kevin@… added
  • Keywords close removed

Replying to scribu:

-1. We should instead make parent_dropdown() use get_posts() instead.

This would require that the 'fields' query var (#14777) accept an array and not just 'all' or 'ids'.

Is there enough consensus on that idea to make such a patch likely to succeed?

Some other get_posts() conversion candidates (#17022, #17023, #17025) require DISTINCT or GROUP BY clauses. Are those also fair game to pass as get_posts() args?

#4 @sillybean
6 years ago

I'd suggest deprecating parent_dropdown() in favor of wp_list_pages() with the filters proposed in #8592.

#5 @wonderboymusic
6 years ago

  • Keywords needs-refresh added

This should use get_posts()

@wonderboymusic
6 years ago

#6 @wonderboymusic
6 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Awaiting Review to 3.7

17026.diff switches the direct SQL to get_posts() and adds a 'parent_dropdown_args' filter to the array passed to it

#7 @nacin
6 years ago

Looks good.

#8 @SergeyBiryukov
6 years ago

I see two issues with 17026.diff:

  1. It only returns 5 top pages by default, where as the current query has no limit.
  2. If we fix point 1, it's less scalable, since there's no way to only request ID, post_parent, and post_title fields using get_posts() without additional filtering. It'll be requesting all the fields for all pages.

I guess this should wait until we allow the fields parameter to be an array, as noted in comment:1. If that isn't feasible for some reason, perhaps we should filter $items instead?

#9 @wonderboymusic
6 years ago

  • Keywords needs-refresh added

point 1 - yeah, need to not limit
point 2 - get_posts() caches, direct SQL does not, so I disagree

#10 @nacin
6 years ago

I agree with being overly cautious about adding too much of a memory footprint to parent_dropdown().

#11 @nacin
6 years ago

  • Milestone changed from 3.7 to Future Release

There's a lot going on here.

#12 @boonebgorges
5 years ago

  • Keywords needs-refresh removed

The performance differences are not huge. I jury-rigged a database so that parent_dropdown() would generate about 10,000 items. Execution time and memory footprint with 17026.2.diff are maybe 10-15% more than the direct query. I squeezed a little extra performance out with no_found_rows=true and by turning off cache priming for terms/meta.

@boonebgorges
5 years ago

#13 @wonderboymusic
4 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

If you want to look at this, you were the last comment. So, hot potato.

#14 @boonebgorges
4 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

I don't have an appetite for this change. It's likely to cause problems for installs that that have posts with large post_content. "Best" solution would be to have a way to fetch post data that includes post_title but not post_content, but this is a pretty significant change to support an unused function.

Note: See TracTickets for help on using tickets.