WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 8 years ago

Last modified 8 years ago

#12212 closed defect (bug) (wontfix)

post__in and other array_key can't manage empty array properly

Reported by: lifeless85 Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 2.9.1
Component: Query Keywords: has-patch close
Focuses: Cc:

Description

There's a conceptual mysfunction in
http://core.trac.wordpress.org/browser/trunk/wp-includes/query.php#L1723
if we want posts that are included in an empty array we should have obviusly no posts.
Instead by adding conditions to the query, if we have an empty array we add no conditions and the query is still the same ...
i think it's a major issue.

Attachments (1)

query.php.diff (933 bytes) - added by lifeless85 10 years ago.
a patch to fix postin bug

Download all attachments as: .zip

Change History (29)

#1 @lifeless85
10 years ago

i think a good soludion should be changing

elseif ( $q['post__in'] ) {
	$post__in = implode(',', array_map( 'absint', $q['post__in'] ));
	$where .= " AND {$wpdb->posts}.ID IN ($post__in)";
}

to

elseif ( $q['post__in'] ) {
	if ( empty( array_map( 'absint', $q['post__in'] ))){
		$where .= " AND {$wpdb->posts}.ID = NULL
	}
	else {
		$post__in = implode(',', array_map( 'absint', $q['post__in'] ));
		$where .= " AND {$wpdb->posts}.ID IN ($post__in)";
	}
}

#2 @lifeless85
10 years ago

Opz i've left out some code...

$where .= " AND {$wpdb->posts}.ID = NULL";

#3 follow-up: @chrisscott
10 years ago

The problem with specifying an empty array for post__in is that fill_query_vars() sets $q['post__in'] to an empty array if it isn't specified in the query so the elseif statement never evals to true.

It seems to me like this doesn't need to be changed and instead you should be handling passing an empty array to posts__in in your code that calls the query.

#4 in reply to: ↑ 3 @lifeless85
10 years ago

Replying to chrisscott:
Ok, so this is not a solution, but, i think that handling this in my teplate / plugin / widget leads to misunderstanding.
You can't just tell me it's hard to fix so it's right... :)
Remember code is poetry...
I'll be thinking about this ...

#5 follow-up: @scribu
10 years ago

  • Priority changed from high to normal
  • Severity changed from major to normal

You can use $wp_query->set_404();

#6 in reply to: ↑ 5 @lifeless85
10 years ago

Replying to scribu:

You can use $wp_query->set_404();

yes, tnx for your advice!

But i still need to know if the array is empty due to fill_query_vars() or if it was setted and empty, before to use $wp_query->set_404();

Maybe we can use it inside of fill_query_vars(), what's the point of filling query vars when you alredy know it will be no results ... i'll do a test in my test machine ...

#7 @lifeless85
10 years ago

first analysis:
i could set query_vars to NULL instead of empty array in the fill_query_vars() function, if they are alredy setted to empty array.

And then in the get_posts() function change the code to looks like

elseif ( $q['post__in'] === NULL) {
	$this->set_404();
} elseif ( $q['post__in'] ) {
	...
	...
}

but for some reason, inside fill_query_vars all the query_var inside of $array_keys is already setted to an empty array ... i don't get why they are already setted to empty array, if fill_query_vars is supposed to do so ... somebody can explain me this ?

#8 @dd32
10 years ago

The query var is being filled with "empty", The field in question is designed to query posts based on a specified list, If the list is empty, then its not being used.

Instead, You need to not call get_posts() or whatever if you know you dont want to retrieve posts.

If you're hooking into WP_Query for the main post request, you need to modify the query earlier to not run most likely.

If you need any help on those, i'd suggest the wp-hackers mailing list

#9 @lifeless85
10 years ago

no no no ... you didn't get the point...
assume you have this behaviour:

$sticky=get_option('sticky_posts');
$args=array(
	'showposts'=>2,
	'post__in' => $sticky,
	'caller_get_posts' => 1,
	'orderby'=>'meta_value',
	'meta_key'=>'from',
	'order'=>'ASC'
);
include('so-sidebar.php');
$args=array(
	'showposts'=>2,
	'post__not_in' => $sticky,
	'caller_get_posts' => 1,
	'orderby'=>'meta_value',
	'meta_key'=>'from',
	'order'=>'ASC'
);
include('so-sidebar.php');

and inside so-sidebar.php you have a custom loop like this:

<?
$so_query = new WP_Query($args);
if ($so_query->have_posts()) :
	while ($so_query->have_posts()) :
		$so_query->the_post();
?>
<div <?php post_class(); ?> id="post-<?php the_ID(); ?>">
	<h2><a href="<?php the_permalink() ?>"><?php the_title(); ?></a></h2>
	<div class="entry"><?php the_excerpt();?></div>
</div>
<?php
	endwhile;
endif;
?>

OK, now i guess you'll be asking yourself:

why all of this !?!?

The answer is clear.
Because if you use 'orderby'=>'meta_value' to sort your post, you can't have all stiky posts at the top (and sorted too).
So ... this is the frustrating way, i do this.

Now, this is bad, because yes i could do this in my theme:

$sticky=get_option('sticky_posts');
if( empty($stiky) ) {
	//do all the math here
}

But, this is like saing SELECT ALL ITEMS FROM AN EMPTY GROUP the result should be another EMPTY GROUP and now this is not true, this is like swearing against the LOGIC and MATH god who rule us all. you are telling me the same thing chrisscott told me before, this is an hard work, it would be easier to fix it your theme ... not exactly the kind of thing i thought i would have found here...

#10 @dd32
10 years ago

I personally believe it's cleaner for the theme to check before it mindlessly queries for data, esp. if you've got a list that you're wanting to know about.

But, this is like saing SELECT ALL ITEMS FROM AN EMPTY GROUP the result should be another EMPTY GROUP and now this is not true

not exactly the kind of thing i thought i would have found here...

What i'm trying to explain, Is the way that WordPress uses internal query parameters, post__in is designed for a specific purpose, To query the posts provided. Thats it, And while i understand your line of thinking "But its an empty list!, So i dont want anything returned",

If we change that particular query param, here's another dozen which would have to be changed to make them act the same: http://core.trac.wordpress.org/browser/trunk/wp-includes/query.php#L1191 Consistancy between parameters is crucial if its to be memorised by developers, the worst thing is to have to look up "How does this param take its arguements"

I have seen many themes prioritise the sticky posts to the top of the pile, and even display them in a seperate block.

Because if you use 'orderby'=>'meta_value' to sort your post, you can't have all stiky posts at the top (and sorted too). So ... this is the frustrating way, i do this.

Then it might be worth filing a bug on that specifically, That the sticky posts do not abide by the ordering? http://core.trac.wordpress.org/browser/trunk/wp-includes/query.php#L2329

#11 @lifeless85
10 years ago

Ok, i'm insisting on this because, i've alredy discussed on the forum about the other bug months ago, they told me not to fill a bug, told me it was better to manage that by myself on my theme, now i think i'll propose to fix that other bug too.

I belive that even if it's surely convinient now to manage that by myself, this is some kind of thing that will make developer crazy.

So why we have theme tag, custom functions and this kind of thing ? if we think people can manage everything by themself they did not need wordpress at all...

this is not a problem of mine, this is a problem on wordpress code, i alredy know how to work around on this.

There's not a dozen other parameter, the parameter that would be afflicted by this are four!!

category__in, post__in, tag__in, tag_slug__in and the cure is something like a couple of if ...

#12 @lifeless85
10 years ago

i've managed to write a simple and in my opinion acceptable solution:
changing the '''function''' fill_query_vars() in the following way:

foreach ( $array_keys as $key ) {
	if ( !isset($array[$key])) {
		if( $key == 'post__in' ){ continue; }
		$array[$key] = array();
	}
}

Notice that we should also change category__in, tag__in, tag_slug__in
And managing the new meaning of empty array inside of '''function''' &get_posts() like this :

  elseif ( is_array($q['post__in']) && empty($q['post__in']) ) {
	$where = " AND {$wpdb->posts}.ID = NULL";
	$this->set_404();
} elseif ( $q['post__in'] ) {
	$post__in = implode(',', array_map( 'absint', $q['post__in'] ));
	$where .= " AND {$wpdb->posts}.ID IN ($post__in)";
}

#13 @lifeless85
10 years ago

sorry for triple quote around the function word i wanted that in bold but i forgot about the effects of triple parentesis...

#14 @lifeless85
10 years ago

Ah! i also wanted to say, that in my opinion we should not initialize $array_keys to empty array because if we test it later by using elseif ( $q['post__in'] ), an empty array evals to false and an unset variable too.

So we should not initialize them and get advantages in the empty array meaning ...

#15 @lifeless85
10 years ago

  • Keywords category__in tag__in tag_slug__in fill_query_vars array_keys get_posts added
  • Milestone changed from Future Release to 2.9.2
  • Summary changed from post__in can't manage empty array properly to post__in and other array_key can't manage empty array properly

#16 @scribu
10 years ago

  • Keywords needs-patch added; post__in category__in tag__in tag_slug__in fill_query_vars array_keys get_posts removed
  • Milestone changed from 2.9.2 to 3.0

I think a better solution would be for fill_query_vars() to fill postin with 'false' or 'null' rather than not setting it at all.

#17 @lifeless85
10 years ago

ok, can you also explain why ?
what about the other three query vals ? i see you stripped them from the tags, but to me looks like they suffer from same problem ...
I see 2.9.2 has been released ... sorry for setting that milestone... now that we need a patch, how can i help ? i can write it ? i should post it here or i should use some kind of svn ?

#18 @lifeless85
10 years ago

P.s. Setting a variable to null in PHP is equivalent to having it undeclared ...
http://it.php.net/manual/en/language.types.null.php#language.types.null.syntax
and setting it to false could lead to other problems, maybe not here where we expect arrays.

But if this practice hopefully get extended to others query vars, setting a variable to false where we expect for example a boolean, is a very bad practice, we should use isset($var) instead of a simple if($var) to distinguish if it's false because it's undefined or it's false because we wanted it to be false ...

So i exclude false would be a good idea, and null is pointless ...

#19 @scribu
10 years ago

now that we need a patch, how can i help ? i can write it ? i should post it here or i should use some kind of svn ?

Read this:

http://core.trac.wordpress.org/wiki#HowtoSubmitPatches

#20 @lifeless85
10 years ago

  • Keywords has-patch added; needs-patch removed

#21 @lifeless85
10 years ago

i'm not sure comments are appropriate in a patch ... but, hey i'm new to svn! always worked alone, so any advice on svn is greatly appreciated.

#22 @scribu
10 years ago

The WordPress codebase might enable conversation, but it's not meant to be a conversation itself. :)

Please remove the comments from your patch.

@lifeless85
10 years ago

a patch to fix postin bug

#23 @lifeless85
10 years ago

  • Keywords tested added

tested query.php.diff on wp 2.9.2

also removed the comments from the patch.

and ... if i can ask ... how can i test this on trunk if trunk does not work ?
because i've tested there too, but with or without my patch it does not work so i can't figure out if this breack something else ... should i write this or not ? sorry for being so newbye about cooperation ...

#24 @scribu
10 years ago

  • Keywords needs-patch added; has-patch tested removed

If it doesn't work on trunk, it can't be commited.

Try to figure out what's changed from the 2.9 branch. (I don't have my laptop, so I can't help with testing atm)

#25 @lifeless85
10 years ago

  • Keywords has-patch tested added; needs-patch removed

tested on trunk revision 13172 it works

hope this time my keywords are ok ... xD sorry for making you work a lot

#26 @nacin
10 years ago

  • Keywords close added; tested removed
  • Milestone changed from 3.0 to Future Release

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.

#27 @wonderboymusic
8 years ago

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

No comments in 2 years since it was tagged "close"

#28 @SergeyBiryukov
8 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.