Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#37721 closed defect (bug) (fixed)

improve error handling of is_object_in_term in taxonomy.php

Reported by: rpayne7264's profile rpayne7264 Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6.1 Priority: normal
Severity: normal Version: 4.6
Component: Taxonomy Keywords: commit
Focuses: Cc:

Description

Line 4276 in taxonomy.php can return a WP_Error object, which is not checked before calling wp_list_pluck().

Result is a fatal error:

Fatal error: Cannot use object of type WP_Error as array in /wp-includes/functions.php on line 3511

The code should pass the WP_Error object to the calling function.

Change History (9)

#1 @boonebgorges
8 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

Thanks for the report, @rpayne7264. Previously:

  • [34812] first started populating the cache here. So 4.4 would have introduced a bug (not specifically noted here, but related) whereby a WP_Error object might be incorrectly cached. See #32044.
  • [37573] changed the taxonomy relationship cache structure so that only item IDs are stored. This is when the wp_list_pluck() call was introduced. See #36814.

So the notice is a 4.6 regression, but there's a related bug going back to 4.4.

#2 @boonebgorges
8 years ago

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

In 38277:

In is_object_in_term(), return error object rather than caching it.

This change prevents an error object from being stored in the cache,
and prevents notices from being thrown when plucking term IDs to put
into the relationship cache.

See #32044, #36814.

Props rpayne7264.
Fixes #37721.

#3 @boonebgorges
8 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 4.6.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.6.1 and review.

#4 @sc0ttkclark
8 years ago

Starting to run into this on live sites too, where code was calling has_term() for a previously non-existing taxonomy. Of course that's bad practice, running code like that, but leftover code from long ago started fatal erroring a whole site can be scary for folks with less dev skills to track down what was going on :)

Definitely need this in 4.6.1

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


8 years ago

#6 @jeremyfelt
8 years ago

  • Keywords commit added; dev-feedback removed

Looks good. +1 for merge to the 4.6 branch.

#7 @rpayne7264
8 years ago

As an additional precaution, you may want to add a check at the entrance of wp_list_pluck() to ensure that the $list param is, indeed, an array.

This is how I would code it:

function wp_list_pluck( $list, $field, $index_key = null ) {
	$newlist = array();
	if(! is_array( $list ))return $newlist;
	if ( ! $index_key ) {
		/*
		 * This is simple. Could at some point wrap array_column()
		 * if we knew we had an array of arrays.
		 */
		foreach ( $list as $key => $value ) {
			if ( is_object( $value ) ) {
				$list[ $key ] = $value->$field;
			} else {
				$list[ $key ] = $value[ $field ];
			}
		}
		return $list;
	}

	/*
	 * When index_key is not set for a particular item, push the value
	 * to the end of the stack. This is how array_column() behaves.
	 */

	foreach ( $list as $value ) {
		if ( is_object( $value ) ) {
			if ( isset( $value->$index_key ) ) {
				$newlist[ $value->$index_key ] = $value->$field;
			} else {
				$newlist[] = $value->$field;
			}
		} else {
			if ( isset( $value[ $index_key ] ) ) {
				$newlist[ $value[ $index_key ] ] = $value[ $field ];
			} else {
				$newlist[] = $value[ $field ];
			}
		}
	}

	return $newlist;
}
Last edited 8 years ago by rpayne7264 (previous) (diff)

#8 @boonebgorges
8 years ago

@jeremyfelt Thanks for the review.

@rpayne7264 Thanks for the suggestion. There has been discussion in the past about this kind of check in wp_list_pluck(), but the general sense is that the PHP notice is a good indicator for developers that they've done something wrong, and should not be silenced. See #35087.

#9 @boonebgorges
8 years ago

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

In 38346:

In is_object_in_term(), return error object rather than caching it.

This change prevents an error object from being stored in the cache,
and prevents notices from being thrown when plucking term IDs to put
into the relationship cache.

See #32044, #36814.

Merges [38277] to the 4.6 branch.

Props rpayne7264.
Fixes #37721.

Note: See TracTickets for help on using tickets.