WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#11737 closed defect (bug) (fixed)

PHP Notice in get_cat_name()

Reported by: miqrogroove Owned by: nacin
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Warnings/Notices Keywords: has-patch tested commit dev-reviewed
Focuses: Cc:

Description

Notice: Trying to get property of non-object in /wp-includes/category.php on line 186

Can be triggered very easily by constructing a URL like /wp-admin/categories.php?action=delete&cat_ID=99999999999

Attachments (2)

11737.diff (548 bytes) - added by nacin 12 years ago.
I chose null here, but false or an empty string might make more sense.
11737.2.diff (586 bytes) - added by nacin 12 years ago.
Empty string makes far more sense

Download all attachments as: .zip

Change History (15)

#1 @nacin
12 years ago

Quick fix:
get_cat_name() should probably check to see if get_category() returns a wp_error, and if so, return (instead of returning ->name for a WP_Error). I'd suggest it passes along the error, but really, if you're asking for the cat name and the cat doesn't exist, you should get an empty string.

Real fix:
Like in wp-admin/comment.php for comments, wp-admin/categories.php should always first check if the category is legit before doing stuff to it. That would be something like if( is_wp_error( get_category($cat_ID) ) ) wp_die('No category with this ID.');

#2 @sirzooro
12 years ago

  • Cc sirzooro added

I think returning WP_Error is better than calling wp_die(), because in this case WP (directly or with plugin help) may return 404 to browser and/or display page "no category found".

#3 @nacin
12 years ago

Turns out it is only WP_Error when the taxonomy doesn't exist. get_category() will return such a WP_Error via get_term(), but if the taxonomy exists but the term doesn't, then a null value is returned.

For get_cat_name(), I think you should be expecting either a name or nothing.

The larger fix I mentioned above doesn't seem to be needed here -- all we need to do is patch get_cat_name(), apparently.

@nacin
12 years ago

I chose null here, but false or an empty string might make more sense.

#4 @miqrogroove
12 years ago

  • Keywords has-patch added

#5 @miqrogroove
12 years ago

  • Keywords tested added

#6 @miqrogroove
12 years ago

Tested using the original test case on 2.9.1. Works much better so far.

#7 @filosofo
12 years ago

I think it should be an empty string returned since this function is used in themes.

#8 @miqrogroove
12 years ago

echo NULL; doesn't cause problems generally, but I agree it would look better to patch without changing the documented return type.

#9 @nacin
12 years ago

Yea, I agree. I had been thinking about this for a bit now. An empty string makes much more sense.

@nacin
12 years ago

Empty string makes far more sense

#10 @miqrogroove
12 years ago

Re-tested 11737.2.diff on 2.9.1 and working perfectly.

#11 @nacin
12 years ago

  • Keywords commit added

#12 @westi
12 years ago

  • Keywords dev-reviewed added
  • Owner changed from westi to nacin
  • Status changed from new to assigned

Looks good to me.

#13 @nacin
12 years ago

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

(In [13074]) Fix notice in get_cat_name(). Return empty string if category does not exist, fixes #11737.

Note: See TracTickets for help on using tickets.