WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#14997 closed enhancement (fixed)

Single filter for all WP_Query clauses

Reported by: scribu Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:

Description

From #14349:

Most times, when you have to manipulate the raw SQL request in WP_Query, you need to alter multiple clauses (JOIN, WHERE etc.).

Currently, you need to register a callback for each of those:

function fictional_alter_where( $where, $wp_query ) {
    if ( $wp_query->get( 'custom_var' ) ) {
        $where .= " AND meta_value IN ( 'foo', 'bar' )";
    }

    return $where;
}
add_filter( 'posts_where', 'fictional_alter_where' );

function fictional_alter_join( $join, $wp_query ) {
    global $wpdb;

    if ( $wp_query->get( 'custom_var' ) ) {
        $join .= " JOIN $wpdb->postmeta ON ($wpdb->posts.ID = $wpdb->postmeta.post_id )";
    }

    return $join;
}
add_filter( 'posts_where', 'fictional_alter_join' );

I think we can all agree that this is pretty cumbersome.

Here's how it can look like:

function fictional_alter_query( $bits, $wp_query ) {
    global $wpdb;

    if ( $wp_query->get( 'custom_var' ) ) {
    	$bits['where'] .= " AND meta_value IN ( 'foo', 'bar' )";
        $bits['join']  .= " JOIN $wpdb->postmeta ON ($wpdb->posts.ID = $wpdb->postmeta.post_id )";
    }

    return $bits;
}
new WP_Query_Manipulation( 'fictional_alter_query' );

WP_Query_Manipulation is a class that leverages the existing hooks, but it has a lot of overhead and isn't 100% reliable.

So, I propose we add this filter in WP_Query.

Attachments (8)

wp_query_bits.diff (3.0 KB) - added by scribu 6 years ago.
wp_query_clauses.diff (3.1 KB) - added by scribu 6 years ago.
14997-notices.patch (2.9 KB) - added by hakre 6 years ago.
14997.patch (4.5 KB) - added by hakre 6 years ago.
"posts_clauses" and "posts_clauses_request" single filter for all clauses (standard and caching plugins)
14997-extract-if-complete.patch (4.3 KB) - added by hakre 6 years ago.
14997-extract-if-complete.2.patch (4.3 KB) - added by hakre 6 years ago.
Against current trunk
14997.diff (1.4 KB) - added by nacin 6 years ago.
14997-notices.2.patch (1.1 KB) - added by hakre 6 years ago.

Download all attachments as: .zip

Change History (43)

#1 @mikeschinkel
6 years ago

  • Cc mikeschinkel@… added

Props!

@scribu
6 years ago

#2 @scribu
6 years ago

While coming up with a patch, I noticed the 'posts_*_paged' filters. These were introduced in [2000], back when WP had something called 'paging by days':

http://core.trac.wordpress.org/browser/trunk/wp-includes/classes.php?annotate=blame&rev=2000#L482

#3 @scribu
6 years ago

  • Keywords has-patch added
  • Summary changed from Single filter for all query bits in WP_Query to Single filter for all WP_Query clauses

#4 @hakre
6 years ago

Patch looks nice to me. I first would have argumented, that it's possible to create a class and handle it therein for a plugin, but the filter introduced with the patch looks nice as well. Should save plugin authors some time to code something based on the SQL commands.

Making SQL queries the other command pattern next to the HTTP query.

#5 @hakre
6 years ago

After thinking about the suggested patch a bit, I was wondering how to deal with unset variables. Those are local variables that are unset prior to the patch to whitelist them for changes (EXTR_SKIP). If a filter now unsets array keys of the same name, those value will stay unset in the function but they are expected to be existing.

#6 follow-up: @scribu
6 years ago

That will be easy to spot if you have WP_DEBUG on.

#7 in reply to: ↑ 6 @hakre
6 years ago

Replying to scribu:

That will be easy to spot if you have WP_DEBUG on.

Well that's nice. Being lazy on the implementation and leaving the work to debuggers.

I think you didn't get my point. It's not about whether or not unset variables are able to spot.

It's about the matter of fact that the filter should not be able to break the data in the function in such an elementary way (unsetting local function variables). Just making my mind about the constraints of the suggested implementation.

But I have no clue how to rate deleted array keys when the data get's back into that function. Empty string? The original string? Does there need to be a "where" inside $where? I was just giving note on the issue which went through my head.

#8 @scribu
6 years ago

If the developer unsets a clause, the query should fail. The only thing we could do is trigger a warning instead of a notice.

#9 @scribu
6 years ago

  • Milestone changed from Awaiting Review to 3.1

#10 @scribu
6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [15775]) Introduce wp_query_clauses filter. Fixes #14997

#11 @hakre
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Patch produces warning/notices about unset variables, if a filter removes keys from the array.

#12 @scribu
6 years ago

Yes, see #comment:8

#13 @hakre
6 years ago

Please define "query should fail". Working on a patch.

#14 @scribu
6 years ago

Again, I don't think explicit handling of this edge case is useful.

Anyway, by failing I mean it should return an empty array.

@hakre
6 years ago

#15 follow-up: @hakre
6 years ago

First patch that deals with notices attached. Removed variables fall back on their default values.


Two questions:

  1. Do caching plugins not deserve a "all clauses at once" filter?
  2. Why it the filter named wp_query_clauses and not "posts_clauses"?

#16 @hakre
6 years ago

Second notice patch with two single-type filters: posts_clauses which is for normal plugins (and is the one Nacin named wp_query_clauses in his patch) and posts_clauses_request as second single-type filter for caching plugins (not in Nacin's patch).

As Nacin already stated, the filter names are inconsistently named but I have no real idea how to deal with that w/o breaking things.

@hakre
6 years ago

"posts_clauses" and "posts_clauses_request" single filter for all clauses (standard and caching plugins)

#17 @nacin
6 years ago

You probably mean scribu.

#18 in reply to: ↑ 15 ; follow-up: @scribu
6 years ago

Replying to hakre:

First patch that deals with notices attached. Removed variables fall back on their default values.

That will cause more confusion than notices. Suppose you have two filters:

The first one modifies JOIN and WHERE.

The second one unsets the JOIN clause.

You end up with an SQL error because the JOIN clause (default) no longer contains the tables needed for the (modified) WHERE clause.

#19 in reply to: ↑ 18 @hakre
6 years ago

Replying to nacin:

You probably mean scribu.

Yes, I wrote nacin but meant scribu. Too late, yester, sorry for wrong crediting here.


Replying to scribu:

Replying to hakre:

First patch that deals with notices attached. Removed variables fall back on their default values.

That will cause more confusion than notices. Suppose you have two filters:

The first one modifies JOIN and WHERE.

The second one unsets the JOIN clause.

You end up with an SQL error because the JOIN clause (default) no longer contains the tables needed for the (modified) WHERE clause.

The problem you describe is some other problem, it's about SQL verification which we don't do. It is unrelated to your or my patch. You wrote yourself "query should fail" in such a case. As it does. Did I read you wrong? Please provide more detail then.

Code should not do notices. So I fixed the code.

Questions:

  1. Should the SQL query be syntactically verified?
  2. Should not be cared about notices any longer? (that would be new to me)
  3. Should an error be raised if a filter unsets/removes a clause part?

#20 follow-up: @scribu
6 years ago

Code should not do notices. So I fixed the code.

Code should not do notices, with default theme and no plugins.

For example, if a function expects 3 parameters, but non-core code passes only 2, it will trigger an error (warning or notice, doesn't matter). That's ok.

  1. Should the SQL query be syntactically verified?

No.

  1. Should not be cared about notices any longer? (that would be new to me)

See above.

  1. Should an error be raised if a filter unsets/removes a clause part?

Maybe.

#21 in reply to: ↑ 20 @hakre
6 years ago

Replying to scribu:

  1. Should not be cared about notices any longer? (that would be new to me)

See above.

Probably that question was too unconcrete, I need to ask more:

2.1. Previous to the done commit, were filters able to break the SQL syntax without giving notices?

2.2. You classified it a problem that a filter can break the SQL syntax without giving notcies. If so, do you think it's wished to give notices now when a SQL related clause filter breaks the SQL syntax?

  1. Should an error be raised if a filter unsets/removes a clause part?

Maybe.

3.1. Can you name reason(s) for the cases under which circumstances an error should be raised?

3.2. can you name reason(s) for the cases an error should not be raised?

#22 @scribu
6 years ago

The goal isn't to not have notices, but to warn developers when they make a mistake.

So, if you want to help, please make a patch that does this: instead of throwing a notice when a clause is unset, it throws a warning to the effect of Hey, you unset the 'where' clause by mistake, which hopefully will be seen even if WP_DEBUG is turned off.

2.1. Previous to the done commit, were filters able to break the SQL syntax without giving notices?

Yes.

2.2. You classified it a problem that a filter can break the SQL syntax without giving notcies. If so, do you think it's wished to give notices now when a SQL related clause filter breaks the SQL syntax?

No, SQL errors are handled by $wpdb.

#23 @nacin
6 years ago

Two things I'm noticing here.

One, I don't know if I like that the order of the filters was changed. I have previously used the posts_where filter to add a callback to posts_join if necessary. While those haven't swapped locations, other individuals could have done something that is based on the order of operations as they had been applied. It's purely for readability purposes -- it's not necessary to move them.

Two, the logic there could be improved by using compact(). At most, I would suggest the following: compact() the arguments, then before extracting them, check that each index is set. If not all are set, then refuse to unset/extract. Otherwise, unset and extract.

#24 @scribu
6 years ago

One, I don't know if I like that the order of the filters was changed. I have previously used the posts_where filter to add a callback to posts_join if necessary.

That's precisely the kind of hacks the new filter aims to address. Anyway, I will revert to the original order, for back-compat.

#25 @scribu
6 years ago

(In [15791]) Restore previous filter order in WP_Query. See #14997

#26 @hakre
6 years ago

Patch against r15776, feedback by nacin, order of previous filters restored and extracting only in case all variable names are preserved by the single filter(s).

@hakre
6 years ago

Against current trunk

#27 @scribu
6 years ago

Just for the record, I only saw 14997.patch now.

Unlike the first patch, unset clauses were set to an empty string, which was ok.

#28 follow-up: @scribu
6 years ago

(In [15794]) Add posts_clauses_request filter too. Props hakre. See #14997

#29 @scribu
6 years ago

I commited a refreshed version of 14997.patch because it's consistent to what would happen if you would return NULL in one of the individual filters.

@nacin
6 years ago

#30 in reply to: ↑ 28 @westi
6 years ago

Replying to scribu:

(In [15794]) Add posts_clauses_request filter too. Props hakre. See #14997

This change is bad is so many ways.

Some times it is better to repeat yourself so the code is easy to understand and self documenting.

#31 @scribu
6 years ago

Yes, nacin and rboren were already all over me on IRC. :)

#32 @scribu
6 years ago

(In [15795]) Sacrifice DRY for readability in WP_Query. Props nacin for initial patch. See #14997

#33 @hakre
6 years ago

[15795] gives notices in case filter returns not an array, e.g. false.

#34 @nacin
6 years ago

There are plenty of times where if a plugin returned incorrect data, WP would be confused and in many cases we don't do sanity checks. I think it's apparent that we need an array back here. Shall we add a cast?

#35 @markjaquith
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [16576]) Cast to array, to avoid notices. props hakre. fixes #14997

Note: See TracTickets for help on using tickets.