WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#22061 closed defect (bug) (fixed)

If $this->posts is empty, don't do stuff to it

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

Description

I started noticing close to 1000 of these a day in our logs: Error: Invalid argument supplied for foreach(). Googlebot is the culprit for the requests, but the error is firing because $this->posts in WP_Query::get_posts() can be an empty array, but that won't stop us from iterating over it.

$this->post_count = count( $this->posts );

// Always sanitize
foreach ( $this->posts as $i => $post ) {
	$this->posts[$i] = sanitize_post( $post, 'raw' );
}

if ( $q['cache_results'] )
	update_post_caches($this->posts, $post_type, $q['update_post_term_cache'], $q['update_post_meta_cache']);

if ( $this->post_count > 0 ) {
	$this->post = $this->posts[0];
}

My patch tucks the foreach and update_post_caches into the final IF statement

Attachments (2)

empty-this-posts.diff (915 bytes) - added by wonderboymusic 8 years ago.
22061.diff (1.1 KB) - added by wonderboymusic 7 years ago.

Download all attachments as: .zip

Change History (11)

#1 @DrewAPicture
8 years ago

Makes good sense. +1

#2 @wonderboymusic
8 years ago

  • Version set to 2.9

Introduced in [12062]

#3 @dd32
8 years ago

Makes sense, but, iterating over an empty array doesn't cause that notice, no notice will be given for an empty array. That type of notice will be issued if you pass a non-array/non-iterable value to foreach - for example, false.

#4 @wonderboymusic
8 years ago

Yeah, I think it is null in some places

#5 @ryan
8 years ago

I've seen it be an empty string too, usually because of plugins. I believe count() returns 1 for empty string.

#6 @ryan
8 years ago

posts being an empty string led to [22011].

#7 @nacin
8 years ago

  • Keywords reporter-feedback added

Normally, if you try to nuke the main query by getting null to be passed to $wpdb->get_results(), then you also need to hook into posts_results to properly set $this->posts again. posts_results happens just after [22011]. This is how the advance post caching plugin works.

So, on one hand, we should set $this->posts to an empty array, if by some point it is not an array. On the other hand, if it isn't an array by that point, then a developer error occurred, and it probably should continue to blare warnings until it is fixed.

I'd try to track down why it posts is actually not an array by the time we start treating it like one.

@wonderboymusic
7 years ago

#8 @wonderboymusic
7 years ago

  • Keywords reporter-feedback removed
  • Owner set to wonderboymusic
  • Status changed from new to accepted

Counting an empty string equals 1 (thanks, PHP) - so I think we should actually expand what my previous patch had.

Equals 1:

php -r "echo count('') . PHP_EOL;"

Because it equals 1, this would fire too:

if ( $this->post_count > 0 ) {
	$this->post = $this->posts[0];
}

My new patch bails earlier and sets posts to empty array and post_count to 0. Also passes all Unit Tests.

#9 @ryan
7 years ago

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

In 22465:

If $this->posts is empty, don't do stuff to it.

Props wonderboymusic
fixes #22061

Note: See TracTickets for help on using tickets.