Single filter for all WP_Query clauses

[scribu]

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.

[mikeschinkel]

Props!

[scribu]

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

[hakre]

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.

[hakre]

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.

[scribu]

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

[hakre]

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.

[scribu]

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

[scribu]

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

[hakre]

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

[scribu]

Yes, see #comment:8

[hakre]

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

[scribu]

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]

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"?

[hakre]

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]

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

[nacin]

You probably mean scribu.

[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.

[hakre]

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?

[scribu]

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.

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

See above.

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

Maybe.

[hakre]

Replying to scribu:

2. 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?

3. 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?

[scribu]

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.

[nacin]

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.

[scribu]

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.

[scribu]

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

[hakre]

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]

Against current trunk

[scribu]

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.

[scribu]

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

[scribu]

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.

[westi]

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.

[scribu]

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

[scribu]

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

[hakre]

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

[nacin]

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?

[markjaquith]

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