WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#9886 closed enhancement (fixed)

Pass $this as the second argument in WP_Query filters

Reported by: westi Owned by: westi
Milestone: 3.0 Priority: normal
Severity: major Version: 2.8
Component: Plugins Keywords: has-patch tested commit early
Focuses: Cc:

Description

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.

Attachments (5)

9886.diff (6.9 KB) - added by scribu 4 years ago.
apply_filters_ref_array.test.php (670 bytes) - added by scribu 4 years ago.
apply_filters_ref_array.diff (2.0 KB) - added by scribu 4 years ago.
Introduce apply_filters_ref_array()
apply_filters_ref_array.2.diff (9.4 KB) - added by scribu 4 years ago.
Use in wp-includes/query.php
apply_filters_ref_array.test.2.php (516 bytes) - added by scribu 4 years ago.
Test filters in query.php

Download all attachments as: .zip

Change History (33)

comment:1 westi5 years ago

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

comment:2 follow-up: Denis-de-Bernardy5 years ago

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

comment:3 in reply to: ↑ 2 westi5 years ago

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.

comment:4 Denis-de-Bernardy5 years ago

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.

comment:5 westi5 years ago

Yeah I think we need a apply_filter_ref_array

comment:6 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.8 to Future Release
  • Version set to 2.8

punting pending patch

comment:7 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9
  • Severity changed from normal to major
  • Summary changed from Cannot reliably add query arguments to all WP_Query objects to Add an apply_filter_ref_array() function
  • Type changed from defect (bug) to feature request

comment:8 westi4 years ago

  • Milestone changed from 2.9 to Future Release

Moving to Future Release for now due to Feature Freeze.

comment:9 westi4 years ago

  • Keywords early added
  • Milestone changed from Future Release to 3.0

Moving into 3.0 I want to fix this one early

comment:10 Denis-de-Bernardy4 years ago

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

comment:11 follow-up: dd324 years ago

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.

comment:12 in reply to: ↑ 11 ; follow-up: Denis-de-Bernardy4 years ago

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.

comment:13 Denis-de-Bernardy4 years ago

  • Keywords 2nd-opinion added

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

comment:14 hakre4 years ago

  • Summary changed from Add an apply_filter_ref_array() function to Add an apply_filters_ref_array() function

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.

comment:15 in reply to: ↑ 12 scribu4 years ago

  • Keywords 2nd-opinion removed

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.

comment:16 scribu4 years ago

  • Summary changed from Add an apply_filters_ref_array() function to Pass $this as the second argument in WP_Query filters

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.

scribu4 years ago

comment:17 scribu4 years ago

  • Keywords has-patch commit added; needs-patch removed
  • Type changed from feature request to enhancement

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

comment:18 follow-up: westi4 years ago

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

comment:19 follow-up: filosofo4 years ago

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?

comment:20 in reply to: ↑ 19 scribu4 years ago

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.

comment:21 in reply to: ↑ 18 ; follow-up: scribu4 years ago

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.

comment:22 in reply to: ↑ 21 westi4 years ago

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.

comment:23 westi4 years ago

  • Keywords commit removed

scribu4 years ago

Introduce apply_filters_ref_array()

comment:24 scribu4 years ago

  • Keywords changed from has-patch early to has-patch early

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.

scribu4 years ago

Use in wp-includes/query.php

scribu4 years ago

Test filters in query.php

comment:25 scribu4 years ago

  • Keywords tested commit added

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

comment:26 dd324 years ago

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

comment:27 nacin4 years ago

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

comment:28 automattor4 years ago

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

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

Note: See TracTickets for help on using tickets.