Make WordPress Core

Opened 9 years ago

Last modified 5 months ago

#33341 reopened defect (bug)

WP_Meta_Query IN operator with empty array does not return expected result

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: critical Version: 3.2
Component: Query Keywords: dev-feedback
Focuses: Cc:

Description

When using the IN compare mode in a WP_Meta_Query where the value is an empty array, this query is evaluated with an unexpected behavior.

For example, I have some posts with a meta field called 'some-textfield':

$posts = get_posts( array(
	'posts_per_page' => -1,
	'post_type' => 'post',
	'post_status' => 'publish',
	'meta_query' => array(
		array(
			'key'	 => 'some-textfield',
			'value' => array(),
			'compare' => 'IN',
		),
	),
) );

This code returns all posts although I would rather expect it to return no posts at all since the value of 'some-textfield' (whatever it may be) is not in those provided in the value field.

I discovered it when I needed to perform a meta query where the value was an array that was returned by a previous operation. It is obviously a minor issue since we can simply check if the array is empty (and in that case do not make the query at all) - but I thought it's not really the expected result of such a query.

The solution would be to:

  • ignore this condition if the relation is set to OR and there are other conditions available
  • evaluate this condition to false otherwise (and thus return no results)

Change History (7)

#1 @helen
9 years ago

  • Keywords reporter-feedback added
  • Version changed from trunk to 3.2

Hi flixos90 - welcome to Trac, and thanks for the detailed report. My expectation in this case would be that the condition is discarded, which in your example would indeed lead to all posts being returned. Does it currently work per the first part of your solution, ignoring/discarding that relation but honoring the others?

#2 @flixos90
9 years ago

  • Keywords dev-feedback added; reporter-feedback removed

Hi Helen, yes that works indeed, so it is not really a functional problem. It probably depends on the point of view, but I personally would not expect the condition to be discarded. I would think that it is processed like the other conditions, causing the query (in my case) to return no posts at all.

As far as I checked in core, the query is sent to MySQL, but it is basically being discarded by MySQL, so it does not happen directly in core. But the behavior is, like I said, just not the one I was expecting - why would any condition in a meta query be discarded? I would propose to manually work around that in the code by ignoring the condition only if it happens within an 'or' relation with at least one more condition provided (since it is not relevant in this case), but otherwise by returning an empty array immediately (since this condition does not apply to any post).

#3 @boonebgorges
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I agree with flixos90 that an empty array should be interpreted so that no records are matched.

The problem is that the WordPress never handles IN params like this. See post__in, post_parent__in, various include params (like get_terms()), and so forth. All of these are processed like this:

if ( ! empty( $include ) ) {
    // ...
    $sql[] = "ID NOT IN {$include_sql}";
}

Your suggestion would be something like:

if ( is_array( $include ) || ! empty( $include ) ) { // ...

But if we change the expected behavior in one place, we have to change it everywhere. And we can't do this, as it'll surely break lots of plugins.

As a workaround, I suggest that your code converts clauses like the one you suggest to something like this when it's determined that the value array is empty:

array(
    'key' => 'some-textfield',
    'compare' => 'NOT EXISTS',
)

More generally, use array( 0 ) for post__in, include, etc. Not ideal, but the best we can do given what we've got.

(I know there have been other tickets where this issue has been discussed, but I can't find them right now.)

Thanks for the ticket, flixos90 :)

#4 @flixos90
9 years ago

Yeah that's true, in terms of backwards compatibility this change is definitely not justifiable. Thanks for the feedback anyway!

#5 @patkemper
3 years ago

  • Resolution wontfix deleted
  • Severity changed from normal to critical
  • Status changed from closed to reopened

Well, it's have been several years now and this is still an issue.

I don't know, whether it's just me, but this bug is critical, as it causes a severe security issue. As you can see, WordPress isn't handling empty arrays properly, which causes the query to fetch the full dataset.

This faulty behavior might get you in big trouble, if you are not aware of this.
Example situation: Imagine saving sensitive data using a custom post type. Associate some of them to specific users. Users who have just no assiciation might see all the data.

For anyone, who needs a quick fix. This will set the value of the meta-query to [-1], if the value is empty:

<?php
function custom_query($query) {
  $the_meta_query = $query->get( 'meta_query' );
  if( is_array( $the_meta_query ) ) {
      foreach( $the_meta_query as $id => $meta_query ) {
          if ( isset( $meta_query[ 'compare' ] ) && isset( $meta_query [ 'value' ] ) ) {
              if ( $meta_query[ 'compare' ] == 'IN' ) {
                  if ( empty( $meta_query[ 'value' ] ) ) {
                      $the_meta_query[ $id ][ 'value' ] = [ -1 ];
                      $query->set( 'meta_query', $the_meta_query );
                  }
              }
          }
      }
  }   
}

add_action( 'pre_get_posts', 'custom_query' );

#6 @SergeyBiryukov
23 months ago

  • Milestone set to Awaiting Review

#7 @epeleg
5 months ago

17 months (or 8 Years) later and this bug is still Awaiting Review.

I don't see how this makes sense in any way.

when someone ends up discovering this bug it is not difficult to fix (but it might very well be that the developer would not discover the bug but it will show up with the users.

I can see how in some scenarios it might make sense to have this semantics in A UI,
like consider having a list of objects with a shape and color properties,
and the UI asks you what colors you want to choose and you check RED and BLUE to limit to show only RED and BLUE than not checking any color (i.e. having the group of checked items empty) would mean that you do not want to filter and you want to return all the object regardless of their color.

BUT this does not make sense when writing code. none of the shapes has a color that is a IN an empty group of color names.

I would claim that the expected behavior would be that even if the shape has a null value, its value not IN an empty array of values and it would require an array with an element that is null to return the posts that have null in said field.

I doubt that there is lots of code that relays on this odd behavior - had I written such a UI as I described above I would probably make sure that if no colors where selected I would not add a limiting IN with an empty array.

This should be fixed. IMHO if a developer in the past expected IN {} to be true for all it would make more sense for his code to fail that for that of any future developers that would exped IN {} to be false...

At the minimum this should be highlighted in the docs and possibly some warning and an option to explicitly opt-in to one of the two behaviors could be added.

Note: See TracTickets for help on using tickets.