WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#16742 closed defect (bug) (fixed)

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

Reported by: 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 6 years ago.
16742.class.diff (11.3 KB) - added by scribu 6 years ago.
Introduce WP_Meta_Query
16742.hash.diff (814 bytes) - added by greuben 6 years ago.
16742.2.diff (2.9 KB) - added by ryan 6 years ago.
query_vars_changed
16742.3.diff (3.0 KB) - added by ryan 6 years ago.
Clean up comments in .2.diff

Download all attachments as: .zip

Change History (29)

#1 @greuben
6 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
6 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
6 years ago

#3 @greuben
6 years ago

  • Keywords has-patch added

#4 @scribu
6 years ago

  • Milestone changed from Awaiting Review to 3.1.1

#5 @scribu
6 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
6 years ago

Introduce WP_Meta_Query

#6 follow-up: @scribu
6 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
6 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
6 years ago

#8 @scribu
6 years ago

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

#9 @scribu
6 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 6 years ago by scribu (previous) (diff)

#10 @scribu
6 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
6 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
6 years ago

  • Milestone changed from 3.1.1 to 3.2

#13 @nacin
6 years ago

Is this not a regression?

#14 @scribu
6 years ago

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

#15 @nacin
6 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
6 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
6 years ago

query_vars_changed

#17 @ryan
6 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
6 years ago

Clean up comments in .2.diff

#18 @ryan
6 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
6 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
6 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 query vars 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.

Last edited 6 years ago by ryan (previous) (diff)

#21 follow-up: @scribu
6 years ago

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

Version 0, edited 6 years ago by scribu (next)

#22 in reply to: ↑ 21 @ryan
6 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
5 years ago

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

#24 @scribu
5 years ago

Follow-up: #17165

Note: See TracTickets for help on using tickets.