WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 weeks ago

#25429 closed defect (bug) (fixed)

`get_the_category_by_ID` triggers PHP notice on non-existent category

Reported by: ericmann Owned by: wonderboymusic
Milestone: 3.9 Priority: normal
Severity: normal Version: 2.3
Component: Taxonomy Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

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.

Attachments (4)

25429.diff (998 bytes) - added by ericmann 7 months ago.
25429.2.diff (1018 bytes) - added by SergeyBiryukov 7 months ago.
25429.3.diff (1.3 KB) - added by ericmann 7 months ago.
25429.4.diff (1.9 KB) - added by SergeyBiryukov 7 months ago.

Download all attachments as: .zip

Change History (13)

ericmann7 months ago

SergeyBiryukov7 months ago

comment:1 SergeyBiryukov7 months ago

  • Milestone changed from Awaiting Review to 3.7
  • Version changed from 3.6.1 to 2.3

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.

comment:2 ericmann7 months ago

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.

comment:3 follow-up: johnbillion7 months ago

  • Keywords dev-feedback added

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?

Last edited 7 months ago by johnbillion (previous) (diff)

comment:4 in reply to: ↑ 3 ericmann7 months ago

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.

ericmann7 months ago

comment:5 nacin7 months ago

  • Keywords 2nd-opinion added; dev-feedback removed
  • Milestone changed from 3.7 to Awaiting Review

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.

SergeyBiryukov7 months ago

comment:6 SergeyBiryukov7 months ago

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

comment:7 bobbingwide2 months ago

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.

comment:8 SergeyBiryukov2 months ago

  • Milestone changed from Awaiting Review to 3.9

comment:9 wonderboymusic6 weeks ago

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

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.

Note: See TracTickets for help on using tickets.