WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

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

Download all attachments as: .zip

Change History (29)

comment:1 greuben3 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

comment:2 scribu3 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.

greuben3 years ago

comment:3 greuben3 years ago

  • Keywords has-patch added

comment:4 scribu3 years ago

  • Milestone changed from Awaiting Review to 3.1.1

comment:5 scribu3 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
        )
)

scribu3 years ago

Introduce WP_Meta_Query

comment:6 follow-up: scribu3 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.

comment:7 in reply to: ↑ 6 greuben3 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

greuben3 years ago

comment:8 scribu3 years ago

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

comment:9 scribu3 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 3 years ago by scribu (previous) (diff)

comment:10 scribu3 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.

comment:11 greuben3 years ago

  • Keywords needs-codex added

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

comment:12 scribu3 years ago

  • Milestone changed from 3.1.1 to 3.2

comment:13 nacin3 years ago

Is this not a regression?

comment:14 scribu3 years ago

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

comment:15 nacin3 years ago

  • Milestone changed from 3.2 to 3.1.1

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

comment:16 ryan3 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.

ryan3 years ago

query_vars_changed

comment:17 ryan3 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.

ryan3 years ago

Clean up comments in .2.diff

comment:18 ryan3 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

comment:19 ryan3 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

comment:20 ryan3 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 3 years ago by ryan (next)

comment:21 follow-up: scribu3 years ago

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

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

comment:22 in reply to: ↑ 21 ryan3 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.

comment:23 ryan3 years ago

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

comment:24 scribu3 years ago

Follow-up: #17165

Note: See TracTickets for help on using tickets.