Opened 10 years ago
Closed 13 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: | |
Focuses: | Cc: |
Description (last modified by )
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)
Change History (48)
#2
@
10 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Future Release to 4.0
#3
@
10 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from new to closed
In 28613:
#4
@
10 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
@
10 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
What about category__in
, tag__in
, author__in
, tag_slug__in
?
#6
@
10 years ago
28099.2.diff implements the kill switch for those other vars and adds unit tests.
#8
@
10 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...
This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.
10 years ago
#12
@
10 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
@
10 years ago
28099-revert.2.diff reverts all of this and updates the unit tests. Holding off for now though.
#14
follow-up:
↓ 15
@
10 years ago
Another broken example: https://github.com/pods-framework/pods/issues/2228
#15
in reply to:
↑ 14
@
10 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:
↓ 17
@
10 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
@
10 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
@
10 years ago
Replying to wonderboymusic:
Yeah, so this seemed like a much better idea at the time...
Pretty much this. Vote revert.
#19
@
10 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
@
10 years ago
- Milestone 4.0 deleted
- Resolution set to wontfix
- Status changed from reopened to closed
#22
follow-up:
↓ 23
@
10 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
@
10 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 thepost__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
@
9 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
@
9 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.
#27
@
6 years ago
I posted a work around to this issue a couple years ago here:
https://wordpress.stackexchange.com/questions/140692/can-i-force-wp-query-to-return-no-results/140727#140727
Basically if use the code below it will return no results or no post which is what you would expect.
'post__in' => array(0)
#28
@
5 years ago
Instead of having that one fixed 5 years ago and then fixed in all consecutive places we left that bug behind and now it gets bigger and bigger as more and more people rely on it. That's a sad story.
I'm confirming 'post__in' => [0]
as a valid workaround. Now everybody wanting to dynamically load some extra posts or not needs a little switch for that.
'post__in' => empty( $include ) ? [ 0 ] : $include,
#29
@
4 years ago
Just ran into this bug myself. I understand the backwards compatibility argument, but the current behavior makes no sense. It would be far more intuitive if a blank array resulted in no posts.
#30
@
4 years ago
Mannnn lost so much time on this!
'post__in' => empty( $include ) ? [ 0 ] : $include,
is a good fix but still do something WPCore please \o/
#31
@
19 months ago
Makes no sense indeed. Could have been cleaned up years ago but wasn't. And now even the core Query Loop block stepped in this mess... #57822
#32
@
13 months ago
I just noticed today after updating WordPress to Version 6.3 that the post__in
array(0)
hack no longer works, but rather shows all posts just like the empty array. My workaround this time was to change the post_type
to something_that_does_not_exist
in the case that the array given to post__in
would be empty.
I'm betting there's quite a number of sites using the zero array hack that now are showing content their developers don't want some viewers to see, and this could lead to some legal action if it was used to segregate or hide content that's confidential or private to those users or groups.
#33
@
13 months ago
- Resolution wontfix deleted
- Severity changed from normal to major
- Status changed from closed to reopened
@SergeyBiryukov - Can we please get a weigh-in on this due to the WordPress 6.3 regression mentioned above?
This ticket was mentioned in Slack in #core by pikamander2. View the logs.
13 months ago
#35
follow-up:
↓ 37
@
13 months ago
- Resolution set to fixed
- Status changed from reopened to closed
Hello, thanks for the report,
Could you please open a new follow-up ticket for this issue, so we can keep this ticket closed on milestone 6.3. Thanks!
#37
in reply to:
↑ 35
@
13 months ago
- Milestone set to Awaiting Review
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to audrasjb:
Could you please open a new follow-up ticket for this issue, so we can keep this ticket closed on milestone 6.3.
This ticket was not on the 6.3 milestone, the fix was reverted 9 years ago in [28783], see comment:19.
Reopening to see why the workaround mentioned in comment:32 no longer works.
#39
follow-up:
↓ 41
@
13 months ago
If there's any way to rip off the band-aid and make passing 0 actually return 0 posts, I think that would be the best way forward.
I haven't looked at the underlying code so I might be getting the details wrong here but I would think that a way forward to a more sane behavior would be to:
- Temporarily throw a deprecation warning in an upcoming release when 0 is passed, instructing the developer to instead use null or -1 or "" or no parameter if they want posts to be displayed or [0] if they don't and informing them that a future release will change the behavior.
- In a release sometime after that, remove the deprecation warning and make 0 actually return 0 posts.
That approach would still cause some friction but would make for a far better default behavior and would allow actively maintained plugins/themes to update their code.
#41
in reply to:
↑ 39
@
13 months ago
- Keywords 2nd-opinion needs-patch added; has-patch removed
Replying to pikamander2:
If there's any way to rip off the band-aid and make passing 0 actually return 0 posts, I think that would be the best way forward.
...
- Temporarily throw a deprecation warning in an upcoming release when 0 is passed...
- In a release sometime after that, remove the deprecation warning and make 0 actually return 0 posts.
I tend to agree with @dd32's assessment from 13 years ago: https://core.trac.wordpress.org/ticket/12212#comment:10. The fact that non-existing post__in
is filled with an empty array is a quirk in WP_Query.
However what would be the purpose to query posts when you want to not find any? Its the same as driving to the grocery store with a shopping list with zero items on it, then buying nothing and returning empty handed :)
the
post__in array(0)
hack no longer works, but rather shows all posts just like the empty array
Is there a better way to do what you need to do without having to use "a hack" and run a query for zero items?
Thinking this can be seen as a regression of "doing-it-wrong" (unintended) use. Seems the "post__in array(0)
" hack will have to be restored. However thinking a "Doing it wrong" would also be needed there.
#42
follow-up:
↓ 43
@
13 months ago
I haven't determined what code changed here to cause post__in
to fail like that. I just ran tests on WP 6.3 and 'post__in' => [ 0 ]
and that works fine (returns no posts as expected).
Can anyone verify this independently, perhaps @Youdaman has a plugin or theme that is filtering pre_get_posts or something and running something akin to array_filter()
on the post__in
argument?
We might be spinning wheels here.
#43
in reply to:
↑ 42
@
13 months ago
Replying to sc0ttkclark:
We might be spinning wheels here.
Thank you @sc0ttkclark for this, I came on just to say the same thing. The issue that I found yesterday was actually caused by something else, but this only became apparent this morning after running multiple tests. Glad you double-checked this!
False alarm, apologies to everyone for the time wasted! I owe everyone a beer or coffee.
To clarify how I even thought this was an issue in the first place, I was modifying the query args param for a plugin that provides a filter, and I think it was ignoring the post__in
array(0)
as opposed to WP itself doing so.
#44
@
13 months ago
- Keywords 2nd-opinion needs-patch removed
- Milestone Awaiting Review deleted
- Resolution set to wontfix
- Status changed from reopened to closed
@Youdaman I've been there! Many times I'm about to hit submit on a PR or ticket and realize that it was something else in the end 😭
Appreciate you getting this in front of everyone but I'm thankful there's no regression here.
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