WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 7 months ago

#28099 closed defect (bug) (wontfix)

Empty array passed to WP_Query post__in returns posts

Reported by: danielbachhuber Owned by: wonderboymusic
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

When you pass an empty array to post__in, you get the most recent posts.

I'd expect that passing an empty array would produce no results.

#12212 is a previous conversation on the matter with the determination:

Strongly tempted to wontfix based on other comments in this thread, specifically dd32's comment about A) consistency in arguments, and B) "I personally believe it's cleaner for the theme to check before it mindlessly queries for data"... Moving to future release for now, suggested close.

However, instantiating a WP_Query object and using if ( $query->have_posts() ) is a valid, and cleaner, approach in a template.

Attachments (4)

28099.diff (1.2 KB) - added by wonderboymusic 3 years ago.
28099.2.diff (3.5 KB) - added by wonderboymusic 3 years ago.
28099-revert.diff (5.0 KB) - added by wonderboymusic 3 years ago.
28099-revert.2.diff (4.9 KB) - added by wonderboymusic 3 years ago.

Download all attachments as: .zip

Change History (30)

#1 @SergeyBiryukov
3 years ago

  • Description modified (diff)

@wonderboymusic
3 years ago

#2 @wonderboymusic
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.0

This also applies to 'post_parent__in' - both are only checked for truthiness.

We could also check if the user specified one of these arguments as empty and nuke the query. Does that in 28099.diff

#3 @wonderboymusic
3 years ago

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

In 28613:

If post__in or post_parent__in is passed to WP_Query as an empty array, nuke the query. Both vars are currently only checked for truthiness after which they are ignored. Setting these vars at all indicates explicit filtering being desired.

Adds unit test.

Fixes #28099.

#4 @nacin
3 years ago

I'm sure something will break somewhere for this. I don't expect we'll need to back this out later in the 4.0 cycle even with that, but let's get a draft up on make/core for review to at least explain this change.

#5 @nacin
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

What about category__in, tag__in, author__in, tag_slug__in?

#6 @wonderboymusic
3 years ago

28099.2.diff implements the kill switch for those other vars and adds unit tests.

#7 @wonderboymusic
3 years ago

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

In 28664:

After [28613], also kill queries that explicityly pass empty arrays to category__in, tag__in, tag_slug__in, and author__in to WP_Query.

Adds unit tests.
Fixes #28099.

#8 @pavelevap
3 years ago

Theme authors use post__in usually for getting Sticky posts with fallback to recently added posts. See example which stopped working in latest trunk: https://themes.trac.wordpress.org/browser/magazino/1.1.5/index.php#L13
In 3.9.1 there is slider with 10 latest posts, but in current trunk there is no slider. There will be many related problems with themes, I guess...

#9 @nacin
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#10 follow-up: @wonderboymusic
3 years ago

Yeah, so this seemed like a much better idea at the time...

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


3 years ago

#12 @pavelevap
3 years ago

I am not sure how many themes use this practice, I only noticed this one. I like current trunk solution (return nothing - it is logical), but maybe there should be some notice for theme authors?

#13 @wonderboymusic
3 years ago

28099-revert.2.diff reverts all of this and updates the unit tests. Holding off for now though.

#15 in reply to: ↑ 14 @sc0ttkclark
3 years ago

Replying to pavelevap:

Another broken example: https://github.com/pods-framework/pods/issues/2228

That's our problem, we're fixing it, but I still agree with the crux of the this ticket, we were doing it wrong and the expectations of an empty array should be to return nothing, because wouldn't want them to see *everything*.

#16 follow-up: @kovshenin
3 years ago

I think Jetpack's infinite scroll breakage is related to this. See: https://github.com/Automattic/jetpack/issues/685

Infinite scroll is trying to build off of the original query so it's using the $wp_the_query->query_vars array, where post__in is set to an empty array by fill_query_vars(). Jetpack is assuming that the results in $wp_the_query were retrieved using $wp_the_query->query_vars which I think is a fair assumption, even though Jetpack could probably build the query off of $wp_the_query->query instead.

My vote is to revert this.

#17 in reply to: ↑ 16 @dkotter
3 years ago

Replying to kovshenin:

I think Jetpack's infinite scroll breakage is related to this. See: https://github.com/Automattic/jetpack/issues/685

Yep, just spent more time then I'd like to admit tracking down why infinite scroll wasn't working, and it was definitely these changes.

I'd second the vote to revert, though I like the idea behind these changes. Just think it might break other things we're not aware of yet.

#18 in reply to: ↑ 10 @helen
3 years ago

Replying to wonderboymusic:

Yeah, so this seemed like a much better idea at the time...

Pretty much this. Vote revert.

#19 @nacin
3 years ago

As noted in IRC, wonderboymusic has been holding off on the revert because of my suggestion to see what breaks and what reports come in. Even with us being sure we'll revert it, we can often gain a lot of insight into what plugin developers are doing by accidentally deliberately breaking things.

Given that we've already seen reports from PODS, Jetpack, etc., I don't think we'll learn too much more keeping it soaking in trunk. Fine with a revert whenever.

#20 @wonderboymusic
3 years ago

  • Milestone 4.0 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

#21 @wonderboymusic
3 years ago

Reverted in [28783]. Updated unit tests.

#22 follow-up: @pglewis
3 years ago

I still support the spirit of this issue and the proposed fix, FWIW.

This is a case of undefined behavior. The WP_Query docs do not specify the behavior of the post__in parameter if it is set but falsey. Our usage in Pods was based on empirical research; the docs don't say you can use it that way but they also don't specify that you can't. Integrators shouldn't be shocked when something that relies on undefined behavior breaks.

I still think it's fine to change this if it is explicitly documented in the codex how the affected parameters will behave in all situations.

#23 in reply to: ↑ 22 @DrewAPicture
3 years ago

Replying to pglewis:

I still support the spirit of this issue and the proposed fix, FWIW.

This is a case of undefined behavior. The WP_Query docs do not specify the behavior of the post__in parameter if it is set but falsey. Our usage in Pods was based on empirical research; the docs don't say you can use it that way but they also don't specify that you can't. Integrators shouldn't be shocked when something that relies on undefined behavior breaks.

I still think it's fine to change this if it is explicitly documented in the codex how the affected parameters will behave in all situations.

See #25367 for incoming work on documenting individual WP_Query arguments.

#24 @maksbd19
2 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Type changed from defect (bug) to enhancement

I had to developed a plugin that requires querying posts based on their linked posts. I could not find a way to use WP_Query class to take care of this, so I queried for the matched posts separately and passed the matched ones in the WP_Query object. I know I should check matched ids are present before the query but in case of a filter like "pre_get_posts" I had no option but to trick the query_var parameter so that it can never find any post based on provided case. I achieved my result but I strongly believe that there should be a nicer way to achieve the same result than this ridiculously ugly hack.

I request to the community and the experts for a separate class for this parameter may be like WP_Meta_Query so that the query class could be used in even more complicated and advanced cases.

#25 @sc0ttkclark
2 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed
  • Type changed from enhancement to defect (bug)

@maksbd19 You have reopened a completely unrelated ticket, try opening a new ticket on it's own.

Last edited 2 years ago by sc0ttkclark (previous) (diff)

#26 @swissspidy
7 months ago

#39682 was marked as a duplicate.

Note: See TracTickets for help on using tickets.