Pass $this as the second argument in WP_Query filters

[westi]

I have a simple plugin which adds and checks query arguments using the following filters:

add_filter( 'query_vars', array(&$this,'register_query_var') );
add_filter( 'posts_where', array(&$this,'posts_where') ); 
add_filter( 'post_limits', array(&$this, 'post_limits') );

It then uses get_query_var() to see if the new vars are set and modfies post_where and post_limits. Unfortunately for new WP_Query objects the calls to get_query_var() will get the value that was used in the main loop or calls to query_posts.

This makes nested loops hard :-(

I think we need to make the filters like post_where pass the WP_Query object as an extra argument.

[westi]

Example code which shows the bug, put this in a Template file:

        <div id="children">
                <dl>
                <?php query_posts('static=true&posts_per_page=-1&child_of='.$id.'&order=ASC'); ?>
                <?php if(have_posts()) : while (have_posts()) : the_post(); ?>
    			<?php 	$inner_query = new WP_Query("post_type=page&posts_per_page=-1&child_of={$id}&order=ASC");
						while ($inner_query->have_posts()) : $inner_query->the_post(); ?>
    			        	<dt><a href="<?php the_permalink();?>"><?php the_title();?>:</a></dt>
                        	<dd style=""><em><?php the_excerpt(); ?></em></dd>
                <?php endwhile; endwhile; endif; ?>
                </dl>
        </div>

Setup the following Page structure:

# Grandparent
    * Child A
          o Grandchild One
          o Grandchild Two
    * Child B
          o Grandchild Four
          o Grandchild Three

Install my Query Child Of plugin from the plugin repo.

You will get the following incorrect output:

    * Child A
    * Child B
    * Child A
    * Child B

[Denis-de-Bernardy]

can't this work: if ( $this == $wp_the_query ) ?

[westi]

Replying to Denis-de-Bernardy:

can't this work: if ( $this == $wp_the_query ) ?

Not sure what you are suggesting here.

We need to pass the $this to the filters so that the plugin can call $this->blah to access the query it is filtering.

[Denis-de-Bernardy]

I was wondering whether $this was defined or not since it's a method, but yeah, the question makes absolutely no sense. An extra arg is needed indeed. We might want to pass it as a reference, too, as is done in do_action_ref_array.

[westi]

Yeah I think we need a apply_filter_ref_array

[Denis-de-Bernardy]

punting pending patch

[westi]

Moving to Future Release for now due to Feature Freeze.

[westi]

Moving into 3.0 I want to fix this one early

[Denis-de-Bernardy]

@Westi: there is a do_action_ref_array very early on in the WP query. I've no idea of what your plugin seeks to do, but I'm 99% you can check if $wp_query === $wp_the_query at that stage, and enable/disable your hooks accordingly.

[dd32]

but I'm 99% you can check if $wp_query === $wp_the_query at that stage

I can see how that will not work.

Picture the plugin needing to apply to EVERY loop, Now picture it having to apply conditionally to every loop. Theres very little movement for conditional applications, you can mess with add/remove filters, but it becomes messy.

There is at present, no global which you can access for the current WP_Query, Query_posts should be avoided at all costs IMO, and thats the only one which uses $wp_query, if you use WP_Query directly (As you have to do anyway for multiple loops) then you've got no way of knowing what query vars the current loop is using.

[Denis-de-Bernardy]

Replying to dd32:

but I'm 99% you can check if $wp_query === $wp_the_query at that stage

Picture the plugin needing to apply to EVERY loop, Now picture it having to apply conditionally to every loop. Theres very little movement for conditional applications, you can mess with add/remove filters, but it becomes messy.

You're very right. I ran into the issue myself no later than yesterday. The Recent Posts widget actually doesn't override the $wp_query. I've added a temporary workaround, but it's not as good as I'd like it to. That said, there is a do_action_ref_array() early on in the WP_Query stuff. Checking over there might be a good option as well.

[Denis-de-Bernardy]

Another alternative, could of course pass the $wp_query object to each of the filters in the query.

[hakre]

Just reviewed the ticket.

• query_vars filter is not part of the class WP_Query, it's part of WP.
• posts_where filter is part of the class WP_Query (function get_posts()).
• post_limits filter is part of the class WP_Query (function get_posts()).

So for query_vars I do not acutally see how it should apply to class WP_Query. It should apply to class WP.

For the other two cases it might indeed make sense. There already is an do_action_ref_array() function available which might be usefull to consult before implementing the new function.

The name of the new function should be apply_filters_ref_array(). To conform with the naming of the existing apply_filters() function.

[scribu]

I'm +1 on the idea of passing the current query as a second parameter to each filter in WP_Query.

You could then do all sorts of neat things, like adding custom query vars:

$my_query = new WP_Query(array('fav_posts_for_user' => 1));

I'll see if I can make a patch.

Replying to Denis-de-Bernardy:

You're very right. I ran into the issue myself no later than yesterday. The Recent Posts widget actually doesn't override the $wp_query. I've added a temporary workaround, but it's not as good as I'd like it to. That said, there is a do_action_ref_array() early on in the WP_Query stuff. Checking over there might be a good option as well.

See #12320 for the Recent Posts widget issue.

[scribu]

Hm, it seems this approach has already been used for 'posts_search' already: [13037]

I'm changing the name of the ticket, since apply_filters_ref_array() would presumably need $this to be set as the first argument, which would break backwards compatibility big time.

[scribu]

Proposed patch simply passes (by reference, for PHP4 compatibility) $this as the second parameter for each filter in the WP_Query class.

[westi]

Have you tested this on PHP4.

I think we still need to have the apply_filters_ref_array() function - this wouldn't need &$this to be it's first argument.

From memory you need to package the reference up in an array so that you can pass it to a function which doesn't take a reference explicitly - it have a variable length arguments list

[filosofo]

Will setting a variable within an object to point that object itself create garbage collection issues? It sounds sketchy, at least.

Couldn't you just pass the query arguments instead?

[scribu]

Have you tested this on PHP4.

No, I haven't.

Worst case, the callback receives a copy of the object, which means it can only read the properties and not modify them.

I think we still need to have the apply_filters_ref_array() function - this wouldn't need &$this to be it's first argument.

You mean apply_filters_ref_array('filter_name', $value, &$this) ?

I guess that could work. I was worried that you couldn't add extra parameters, but it actually makes more sense this way.

Replying to filosofo:

Will setting a variable within an object to point that object itself create garbage collection issues? It sounds sketchy, at least.

That's exactly what's happening in do_action_ref_array() and there haven't been any issues that I've heard of.

Couldn't you just pass the query arguments instead?

Yes, passing the query_vars array would be the next best thing.

[scribu]

Replying to westi:

From memory you need to package the reference up in an array so that you can pass it to a function which doesn't take a reference explicitly - it have a variable length arguments list

Here's what I've found while looking in wp-includes/plugin.php:

do_action() contains these lines:

if ( is_array($arg) && 1 == count($arg) && is_object($arg[0]) ) // array(&$this)
	$args[] =& $arg[0];

And here is the description for do_action_ref_array():

 * Execute functions hooked on a specific action hook, specifying arguments in an array.
 *
 * @see do_action() This function is identical, but the arguments passed to the
 * functions hooked to <tt>$tag</tt> are supplied using an array.

So, either the docs are wrong or we're using do_action_ref_array() wrong.

[westi]

Replying to scribu:

Replying to westi:

From memory you need to package the reference up in an array so that you can pass it to a function which doesn't take a reference explicitly - it have a variable length arguments list

Here's what I've found while looking in wp-includes/plugin.php:

do_action() contains these lines:

if ( is_array($arg) && 1 == count($arg) && is_object($arg[0]) ) // array(&$this)
	$args[] =& $arg[0];

This looks like do_action had some hacky code in it to do what do_action_ref_array does before that existed.

And here is the description for do_action_ref_array():

 * Execute functions hooked on a specific action hook, specifying arguments in an array.
 *
 * @see do_action() This function is identical, but the arguments passed to the
 * functions hooked to <tt>$tag</tt> are supplied using an array.

So, either the docs are wrong or we're using do_action_ref_array() wrong.

I don't see what is wrong here.

do_action_ref_array takes an array as an argument which can contain one or more references.

This array is passed to call_user_func_array and ends up as a list of normal arguments for the function hooked onto the relevant action.

[scribu]

Introduce apply_filters_ref_array()

[scribu]

apply_filters_ref_array.diff introduces apply_filters_ref_array().

Tested on PHP 5.2.10 and PHP 4.4.9 using apply_filters_ref_array.test.php.

[scribu]

Use in wp-includes/query.php

[scribu]

Test filters in query.php

[scribu]

apply_filters_ref_array.2.diff uses the new function in wp-includes/query.php.

Again, tested on both PHP 5 and 4. See apply_filters_ref_array.test.2.php

[dd32]

scribu: For what its worth, The patch looks good to me, I'll leave this for westi however.

[nacin]

westi in [13756]: Introduce apply_filters_ref_array(). See #9886 props scribu.

[automattor]

(In [13758]) Make use of apply_filters_ref_array() when running the query. Fixes #9886 props scribu.