Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#33947 closed defect (bug) (fixed)

get_comment_class raises a PHP notice for non-registered users

Reported by: walterebert's profile walterebert Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: has-unit-tests has-patch
Focuses: template Cc:

Description

Calling the function get_comment_class raises a PHP notice, if a comment is from a non-registered user:

Notice: Trying to get property of non-object in /srv/www/htdocs/wp-includes/comment-template.php on line 444

I would propose to replace:

if ( $comment->user_id > 0 && $user = get_userdata( $comment->user_id ) ) {

With:

if ( ! empty( $comment->user_id ) && $user = get_userdata( $comment->user_id ) ) {

Attachments (4)

comment-template.php.diff (765 bytes) - added by walterebert 10 years ago.
33947.patch (3.0 KB) - added by walterebert 10 years ago.
Patch + unit test
33947.1.diff (3.5 KB) - added by dipesh.kakadiya 10 years ago.
Fixed PHPCS & Add more unittest for Comment object
33947.2.diff (1.7 KB) - added by dipesh.kakadiya 10 years ago.
Patch Updated - Remove whitespace

Download all attachments as: .zip

Change History (16)

#1 @DrewAPicture
10 years ago

  • Keywords needs-unit-tests needs-patch added

Hi @walterebert, welcome to Trac and thanks for the patch!

It seems like we should actually be checking if the get_comment( $comment_id ) a few lines up is returning a valid comment object. My guess is the notice you're getting is probably from an invalid comment ID being passed and core not bailing if the comment is invalid.

#2 @DrewAPicture
10 years ago

Hint: In this case we'd want to bail early and return an empty array if the comment object is invalid. It would also be nice to have test coverage for this if you'd like to try your hand at adding a unit test.

#3 @walterebert
10 years ago

It makes sense to me, to at least have a "comment" / "comment type" class, instead of no class at all.

I will look into writing a unit test for this.

@walterebert
10 years ago

Patch + unit test

#4 @walterebert
10 years ago

My patch includes:

  1. Verification if a comment is a WP_Comment object.
  2. Unit test for get_comment_class.
  3. Format get_comment_class according to WordPress coding standards

#5 @walterebert
10 years ago

  • Keywords has-patch added; needs-patch removed

@dipesh.kakadiya
10 years ago

Fixed PHPCS & Add more unittest for Comment object

#6 @dipesh.kakadiya
10 years ago

  • Keywords needs-unit-tests removed

#7 @DrewAPicture
10 years ago

  • Keywords has-unit-tests added

Hi @dipesh.kakadiya, as a matter of course, we don't fix coding standards in unrelated code in patches. Can you remove the extra whitespace changes?

Last edited 10 years ago by DrewAPicture (previous) (diff)

#8 @DrewAPicture
10 years ago

  • Milestone changed from Awaiting Review to 4.4

@dipesh.kakadiya
10 years ago

Patch Updated - Remove whitespace

#9 @dipesh.kakadiya
10 years ago

@DrewAPicture

Hi @dipesh.kakadiya, as a matter of course, we don't fix coding standards in unrelated code in patches. Can you remove the extra whitespace changes?

Revise patch as your suggested.

#10 @boonebgorges
10 years ago

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

Thanks for the patches, walterebert and dipesh.kakadiya. I'm going to break the tests up a bit, and then go with the proposed fix.

#11 @boonebgorges
10 years ago

In 34453:

Add a few simple tests for get_comment_class().

Props walterebert, dipesh.kakadiya.
See #33947.

#12 @boonebgorges
10 years ago

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

In 34454:

Bail early when invalid ID is passed to get_comment_class().

This helps to avoid PHP notices later in the function.

Props walterebert, dipesh.kakadiya, DrewAPicture.
Fixes #33947.

Note: See TracTickets for help on using tickets.