`get_the_category_by_ID` triggers PHP notice on non-existent category

[ericmann]

When calling get_the_category_by_ID() with a bad category ID, the function internally triggers a PHP notice. Internally, the function uses get_term(), which can return null. However, the function only checks is_wp_error() and not null, leading to the following notice:

Notice: Trying to get property of non-object in /var/www/wordpress/wp-includes/category-template.php on line 141

This bug was originally reported by TechCrunch.

[SergeyBiryukov]

According to the inline docs, get_the_category_by_ID() returns either a string or a WP_Error object, so returning null might break existing code.

25429.2.diff​ makes sure get_category() returns WP_Error if get_term() returns null, and clarifies the return value. The changes in get_the_category_by_ID() are not needed in that case.

[ericmann]

That works for me. I was more concerned about using functions internally that can return null then immediately trying to access $category->name, which is what triggered the notice.

[johnbillion]

Note that get_tag() just returns the value of get_term(), so it returns null if the tag doesn't exist. Should we match get_tag() for consistency by returning null, or should we return a wp_error as per Sergey's patch?

[ericmann]

Replying to johnbillion:

Should we match get_tag() for consistency by returning null, or should we return a wp_error as per Sergey's patch?

Took some time to think about this, and I agree that since get_tag() returns null, then get_category() should do the same. However, get_the_category_by_ID() erring when get_category() returns null is still a problem, so I've moved Sergey's error call higher up the stack.

Some code in production might already be counting on get_tag()/get_category() returning null for nonexistent categories. Anyone doing an if ( null === get_category( $something ) ) check would see a lot of huge issues if we suddenly start returning WP_Error instead. But since get_the_category_by_ID() will currently break with a bad category, we should do the null check inside there to prevent explosions.

Patch 25429.3 adds Sergey's docblock edits to get_category() and also adds a null check to get_the_category_by_ID(). Method returns aren't changed, but this will fix the PHP notice reported above.

[nacin]

25429.3.diff​ is solid. However, this will cause echo get_the_category_by_ID( $nonexistent_id ) to go from a notice to a fatal error. So this is definitely too late for 3.7, and possibly shouldn't occur at all. Returning '' or null might be better, given this is a function designed for presentation.

I feel like deprecating the oddly named get_the_category_by_ID() could also be a possible action here.

[SergeyBiryukov]

25429.4.diff​ fixes the notice and clarifies the return value for get_category() and get_tag().

[bobbingwide]

As a workaround for this problem I have coded something like

  $category = get_term( $ID, 'category' );
  if ( is_wp_error( $category ) || is_null( $category)  ) {
    $catname = "Invalid category - please Delete ";
  } else {
    $catname = $category->name; 
  }

which is almost exactly the same as I would have had to have written to handle the results from a 'fixed' version of get_the_category_by_ID().

Given that you can get a fatal from echo get_the_category_by_ID( 0 ); then I would support either the deprecation of the function or some further work to make the function more useful as a template tag.

[wonderboymusic]

In 27521:

Avoid a notice in get_the_category_by_ID() when is_wp_error( $term ) is false but $term->name is not set. Clarify the @return value of get_category() and get_tag() which return the same possible types as get_term(), which they wrap.

Props ericmann, SergeyBiryukov.
Fixes #25429.