WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#33947 closed defect (bug) (fixed)

get_comment_class raises a PHP notice for non-registered users

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

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 4 years ago.
33947.patch (3.0 KB) - added by walterebert 4 years ago.
Patch + unit test
33947.1.diff (3.5 KB) - added by dipesh.kakadiya 4 years ago.
Fixed PHPCS & Add more unittest for Comment object
33947.2.diff (1.7 KB) - added by dipesh.kakadiya 4 years ago.
Patch Updated - Remove whitespace

Download all attachments as: .zip

Change History (16)

#1 @DrewAPicture
4 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
4 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
4 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
4 years ago

Patch + unit test

#4 @walterebert
4 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
4 years ago

  • Keywords has-patch added; needs-patch removed

@dipesh.kakadiya
4 years ago

Fixed PHPCS & Add more unittest for Comment object

#6 @dipesh.kakadiya
4 years ago

  • Keywords needs-unit-tests removed

#7 @DrewAPicture
4 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 4 years ago by DrewAPicture (previous) (diff)

#8 @DrewAPicture
4 years ago

  • Milestone changed from Awaiting Review to 4.4

@dipesh.kakadiya
4 years ago

Patch Updated - Remove whitespace

#9 @dipesh.kakadiya
4 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
4 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
4 years ago

In 34453:

Add a few simple tests for get_comment_class().

Props walterebert, dipesh.kakadiya.
See #33947.

#12 @boonebgorges
4 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.