Make WordPress Core

Opened 14 years ago

Closed 12 years ago

#17093 closed enhancement (maybelater)

Validation and context parameter for query filter application

Reported by: kevinb's profile kevinB Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:

Description

One improperly implemented query filter can easily crash a site, and this risk only increases with use of filterable get_posts() calls throughout wp-admin.

In the most common scenario, a posts_where or posts_request filter fails to return a value. Even if the filter was intended for wp-admin but was not added selectively based on is_admin(), the site front end is crashed. It's the plugin's fault, but leaves the end user wondering why WP can't be more robust.

The associated patch eliminates that scenario by checking is_null() before accepting a filtered value. If null, the filter is ignored, with a warning triggered if WP_DEBUG is defined. Since this validation is performed in apply_query_filters() rather than the heavily-used apply_filters(), new overhead is minimal.

Another benefit of a dedicated apply_query_filters() function is the ability to apply filters selectively based on a query_context specified by supplemental get_posts() calls. This will also tend to mitigate the scope of query filtering errors if use of the context parameter becomes standard practice for get_posts() execution and filtering.

Attachments (1)

apply_query_filters.diff (13.1 KB) - added by kevinB 14 years ago.
new plugin API for query filtering :add_query_filter() and apply_query_filters()

Download all attachments as: .zip

Change History (7)

@kevinB
14 years ago

new plugin API for query filtering :add_query_filter() and apply_query_filters()

#2 @kevinB
14 years ago

Note: add_query_filter() and apply_query_filters() are a fully equivalent to add_filter() and apply_filters(), with the validation and context check tacked on.

Query filters added via add_filter() still work and also get return value validation.

#3 follow-up: @nacin
14 years ago

I'm not sure, in terms of processing, how this is much different from a callback checking context on their own. I could imagine many use cases would be to do different things on different contexts. I think better effort would be spent tagging every query in core with a context, than to come up with a new (and strict) API.

#4 in reply to: ↑ 3 @kevinB
14 years ago

Replying to nacin:

I'm not sure, in terms of processing, how this is much different from a callback checking context on their own. I could imagine many use cases would be to do different things on different contexts. I think better effort would be spent tagging every query in core with a context, than to come up with a new (and strict) API.

We're in agreement on tagging core queries with a context. Do I hear a task blessing there?

Agreed that context checking by the caller is a lesser concern. But if you look at it in terms of risk mitigation or damage control, a filter added with a specified context is immediately safer. If the added overhead is minimal, isn't that better for everyone? Otherwise every query in the request depends on the callback's successful returning of an unfiltered value in its out-of-context executions - doable but often unnecessary.

If strictness is a concern, context could be implemented as an array to let filters specify multiple contexts. And filters are always free to implement their own context check by leaving it unspecified.

Anyway, I thought the null validation issue might be compelling enough to bring caller-side context checking in on its coattails :)

#5 @scribu
14 years ago

I'm ok with a safe_apply_filters() function that does null checks, but don't think moving the context check there is a good idea.

#6 @wonderboymusic
12 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

The patch on #17019 adds the query_context var, I am going to suggest close here

Note: See TracTickets for help on using tickets.