Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#16742 closed defect (bug) (fixed)

Meta query doesn't work with $wp_query->set()

Reported by: scribu's profile scribu Owned by:
Milestone: 3.1.1 Priority: normal
Severity: normal Version: 3.1
Component: Query Keywords: has-patch dev-feedback needs-codex
Focuses: Cc:

Description

The following doesn't work, apparently:

add_action( 'pre_get_posts', function( $query ) {
  $query->set('meta_key','oqp_guest_email');
  $query->set('meta_value',$dummy_email);
} );

Source: http://wordpress.org/support/topic/pre_get_posts-problem

Attachments (5)

16742.diff (425 bytes) - added by greuben 13 years ago.
16742.class.diff (11.3 KB) - added by scribu 13 years ago.
Introduce WP_Meta_Query
16742.hash.diff (814 bytes) - added by greuben 13 years ago.
16742.2.diff (2.9 KB) - added by ryan 13 years ago.
query_vars_changed
16742.3.diff (3.0 KB) - added by ryan 13 years ago.
Clean up comments in .2.diff

Download all attachments as: .zip

Change History (29)

#1 @greuben
13 years ago

Also this one generates some invalid sql

add_action( 'pre_get_posts', function( $query ) {
$query->set( 'meta_key', 'test' );
$query->set( 'orderby', 'meta_value' );
return $query;
} );

sql query

SELECT SQL_CALC_FOUND_ROWS wp_posts.* FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'private') GROUP BY wp_posts.ID ORDER BY wp_postmeta.meta_value DESC LIMIT 0, 10

#2 @scribu
13 years ago

I thing we're going to have to store the parsed meta_query in it's own property, like we do for the tax_query.

@greuben
13 years ago

#3 @greuben
13 years ago

  • Keywords has-patch added

#4 @scribu
13 years ago

  • Milestone changed from Awaiting Review to 3.1.1

#5 @scribu
13 years ago

  • Keywords has-patch removed

That will cause duplicate meta queries:

query_posts( array( 'meta_key' => 'foo' ) );

After the final call to _parse_meta_query(), 'meta_query' will be:

Array
(
    [0] => Array
        (
            [key] => foo
        )

    [1] => Array
        (
            [key] => foo
        )
)

@scribu
13 years ago

Introduce WP_Meta_Query

#6 follow-up: @scribu
13 years ago

  • Keywords has-patch added

16742.class.diff is a lot simpler than the diff suggests, but it's still a pretty big change for a point release.

#7 in reply to: ↑ 6 @greuben
13 years ago

Replying to scribu:

16742.class.diff is a lot simpler than the diff suggests, but it's still a pretty big change for a point release.

For 3.1.1 maybe we can use hashing(like tax query) and use WP_Meta_Query in 3.2

@greuben
13 years ago

#8 @scribu
13 years ago

16742.hash.diff doesn't solve the original issue in this bug.

#9 @scribu
13 years ago

Actually, it does fix the original bug, but it stomps the original 'meta_query', if it's set:

query_posts( array(
	'meta_query' => array(
		array( 'key' => 'foo' )
	)
) );
Last edited 13 years ago by scribu (previous) (diff)

#10 @scribu
13 years ago

  • Keywords dev-feedback added

After further testing, it seems to work as expected.

Maybe we should just leave it for 3.2, instead of using clever, yet contrived, workarounds like this.

#11 @greuben
13 years ago

  • Keywords needs-codex added

If moving to 3.2, add codex so that plugin/theme authors are notified of the changes.

#12 @scribu
13 years ago

  • Milestone changed from 3.1.1 to 3.2

#13 @nacin
13 years ago

Is this not a regression?

#14 @scribu
13 years ago

Yes, it is. I've expressed my opinion in #comment:10. Your call.

#15 @nacin
13 years ago

  • Milestone changed from 3.2 to 3.1.1

Only a committer should be making a decision to punt a regression.

#16 @ryan
13 years ago

  • Version set to 3.1

16742.hash.diff seems good for 3.1.1. We can leave this ticket open and consider scribu's fix for 3.2.

@ryan
13 years ago

query_vars_changed

#17 @ryan
13 years ago

That patch may be too much for 3.1.1. It eliminates the multiple md5/serialize calls and sets one query_vars_changed flag. Setting the hash in parse_tax_query() seemed odd now that it is applying to more places.

@ryan
13 years ago

Clean up comments in .2.diff

#18 @ryan
13 years ago

(In [17552]) Parse the meta query again if query vars change. Set a global query_vars_changed flag instead of doing multiple hash creation calls. Props greuben. see #16742 for trunk

#19 @ryan
13 years ago

(In [17553]) Parse the meta query again if query vars change. Set a global query_vars_changed flag instead of doing multiple hash creation calls. Props greuben. see #16742 for 3.1

#20 @ryan
13 years ago

I went ahead with the hash cleanup. Please review thoroughly. The hash is now computed at the end of parse_query() right before the parse_query action. It is computed again in get_posts() after pre_get_posts and fill_query_vars() have run. If the hash changed via the parse_query or pre_get_posts actions then the query_vars_changed flag is set to true so that the tax and meta parsers know to do their thing again. There are no other filters or actions that run between when the hash is recomputed and when the taxonomy and meta queries are parsed again.

Tested with init, pre_get_posts, and parse_query hooks that exercised a variety of meta and tax queries via set() and get_posts() calls on the main query and newly instantiated queries.

Version 0, edited 13 years ago by ryan (next)

#21 follow-up: @scribu
13 years ago

Looking at the current solution, I think it might have been safer to go with 16742.class.diff after all. :)

Last edited 13 years ago by scribu (previous) (diff)

#22 in reply to: ↑ 21 @ryan
13 years ago

Replying to scribu:

Looking at the current solution, I think it might have been safer to go with 16742.class.diff after all. :)

Yeah, the little fix wasn't so little. Though the hash cleanup should benefit further meta query work.

#23 @ryan
13 years ago

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

#24 @scribu
13 years ago

Follow-up: #17165

Note: See TracTickets for help on using tickets.