WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 10 months ago

Last modified 10 months ago

#37291 closed defect (bug) (fixed)

Check for WP_Error before echo in the_tags()

Reported by: michalzuber Owned by: boonebgorges
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: fixed-major
Focuses: Cc:

Description

I got Catchable fatal error: Object of class WP_Error could not be converted to string in /wp-includes/category-template.php on line 1101
on an auto updated site.

Attachments (2)

37291.diff (564 bytes) - added by michalzuber 15 months ago.
Check if WP_Error
37291.2.diff (1.7 KB) - added by boonebgorges 12 months ago.

Download all attachments as: .zip

Change History (17)

@michalzuber
15 months ago

Check if WP_Error

#1 @ocean90
14 months ago

  • Keywords has-patch needs-refresh added
  • Milestone changed from Awaiting Review to Future Release
  • Version trunk deleted

@michalzuber What's the code/message of the WP_Error object?

WordPress uses tabs for indentation. I think the following would be more readable:

$tags = get_the_tag_list( $before, $sep, $after );

if ( is_wp_error( $tags ) ) {
	return '';
}

return $tags;

#2 @dd32
12 months ago

After debugging this on WordPress.com, I believe this deserves fixing much deeper than this as well.

In my case, it seems the issue is that get_the_terms() is returning array( WP_Term, WP_Error, WP_Term ) - note the WP_Error in the middle, which trips up get_term_link() and returns that WP_Error there.

The WP_Error is a invalid_term => Empty Term error, which appears to be caused by get_object_term_cache() calling get_term() on the failure of a wp_cache_get() (which should exist, as it was just set by _prime_term_caches()).

I suspect this is caused by out-of-sync term caches.. but not sure exactly how that's happening.
It does look like get_object_term_cache() isn't designed to handle cases where _prime_term_caches() doesn't cache all the terms.
Based on the code in get_object_term_cache() I'd suggest that maybe get_term() should be called directly rather than wp_cache_get( $term_id, 'terms' ) - and WP_Error instances from that should be floated up the call chain.

cc @boonebgorges re [37573]

#3 @ocean90
12 months ago

#38277 was marked as a duplicate.

#4 @boonebgorges
12 months ago

  • Keywords needs-testing added; needs-refresh removed
  • Milestone changed from Future Release to 4.6.2

@dd32 Yes, your idea is a good one. See 37291.2.diff: if any call to get_term() returns an error object, get_object_term_cache() will bail, returning the WP_Error. get_the_terms() and friends are already built in such a way that the error object will be passed up the chain. Can you read the patch (especially the test, which tries to emulate a situation where get_term() would return an error) to make sure it captures the spirit of your idea?

#5 @dd32
12 months ago

@boonebgorges That looks like it does exactly what I was thinking, I was for some reason having difficulty in writing that same patch while dug in deep. I also assume there's a PHPDoc update to go with that though.

One case I was worried about was where it'd just continuously return a WP_Error when the cache was invalid, however by switching purely to get_term() that would cause it to fall back to the database anyway, correct?

#6 @boonebgorges
12 months ago

One case I was worried about was where it'd just continuously return a WP_Error when the cache was invalid, however by switching purely to get_term() that would cause it to fall back to the database anyway, correct?

Yup, all the caching logic is hidden inside of get_term(). This is better anyway, since it means less exposure of the cache internals.

I also assume there's a PHPDoc update to go with that though.

Great, I'll add this and commit. Thanks!

#7 @boonebgorges
12 months ago

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

In 38776:

Taxonomy: Better error handling when fetching object terms from cache.

Since [37573], get_object_term_cache() has expected term IDs to be
stored in the taxonomy relationship cache. The function would then
reach directly into the 'terms' cache to fetch the data corresponding
to a given term, before returning a WP_Term object. This caused
problems when, for one reason or another, term data was cached
inconsistently:

  • If the 'terms' cache is empty for a given term ID, despite the earlier call to _prime_term_caches(), get_term() would return an error object.
  • If the array of cached term IDs contains an invalid ID, get_term() would return an error object.

We avoid these errors by no longer touching the 'terms' cache directly,
but running term IDs through get_term() and allowing that function to
reference the cache (and database, as needed). If get_term() returns
an error object for any of the cached term IDs, get_object_term_cache()
will return that error object alone. This change ensures that upstream
functions, like get_the_terms(), return WP_Error objects in a
predictable fashion.

Props dd32, michalzuber.
Fixes #37291.

#8 @boonebgorges
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.6.2.

Important note: the @since annotation added in [38776] reads 4.6.2. If there ends up being no 4.6.2 release, that value should be changed to 4.7.0 in trunk.

#9 @dd32
12 months ago

In 38777:

Taxonomy: Avoid a fatal error in the_tags() in the event that get_the_term_list() returns a WP_Error.

Props michalzuber.
See #37291.

#10 @dd32
12 months ago

  • Keywords fixed-major added; has-patch needs-testing removed

[38776] should be backported to 4.6.x, [38777] is fine for 4.7/trunk only.

#11 @boonebgorges
12 months ago

In 38779:

Taxonomy: Specify taxonomy when populating cached object terms.

[38776] introduced a call to get_term() using only the term ID. This
causes problems in cases where shared terms have not been split. Since
we have the taxonomy available, there's no harm in passing it along to
get_term().

Props dd32.
See #37291.

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


10 months ago

#13 @helen
10 months ago

  • Milestone changed from 4.7.1 to 4.7

#14 @helen
10 months ago

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

In 39475:

Docs: Update an @since as there will not be a 4.6.2 before 4.7.

props boonebgorges.
fixes #37291. see [38776].

#15 @helen
10 months ago

In 39476:

Docs: Update an @since as there will not be a 4.6.2 before 4.7.

props boonebgorges.
fixes #37291 for the 4.7 branch. see [38776].

Note: See TracTickets for help on using tickets.