WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 months ago

Last modified 2 months ago

#41846 closed defect (bug) (fixed)

Avoid PHP notice in get_comment_reply_link() for null comment

Reported by: birgire Owned by: whyisjake
Milestone: 5.3 Priority: normal
Severity: normal Version: 2.7
Component: Comments Keywords: has-patch has-unit-tests commit
Focuses: Cc:
PR Number:

Description

When I was working on tests for get_comment_reply_link() I got a PHP notice:

Notice	Trying to get property of non-object ...

with inputs like:

get_comment_reply_link( array( 'depth' => 1, 'max_depth' => 2 ) );

when there's no current global comment available and the input comment is null.

That's because of this part:

$comment = get_comment( $comment );

if ( empty( $post ) ) {
    $post = $comment->comment_post_ID;
}

where there's no validation for $comment.

Attachments (3)

41846.patch (1.5 KB) - added by birgire 2 years ago.
41846.diff (1.4 KB) - added by earnjam 6 months ago.
41846-2.diff (1.3 KB) - added by birgire 3 months ago.

Download all attachments as: .zip

Change History (15)

@birgire
2 years ago

#1 @birgire
2 years ago

  • Keywords has-patch has-unit-tests added

In 41846.patch we added a comment validation, similar to the existing one for the post.

Included is a test.

Last edited 2 years ago by birgire (previous) (diff)

#2 @birgire
2 years ago

Related #38925

@earnjam
6 months ago

#3 @earnjam
6 months ago

  • Milestone changed from Awaiting Review to 5.3

Thanks @birgire! This looks good.

In 41846.diff I just made a few small whitespace CS fixes to the unit test and added the ticket number to the @ticket annotation in the docblock.

I'm going to go ahead and milestone this for the next release.

#4 @davidbaumwald
3 months ago

  • Keywords commit added

#5 @whyisjake
3 months ago

When I run phpunit --group comment I am getting the following error.

1) Tests_Comment_GetCommentReplyLink::test_should_return_null_when_depth_less_than_max_depth_and_comment_null_and_no_current_global_comment
TypeError: Argument 2 passed to PHPUnit\Framework\Assert::assertNull() must be of the type string, null given, called in /tests/phpunit/tests/comment/getCommentReplyLink.php on line 89

/tests/phpunit/tests/comment/getCommentReplyLink.php:89

#6 @whyisjake
3 months ago

  • Keywords commit removed

@birgire
3 months ago

#7 @birgire
3 months ago

@whyisjake thanks for testing

41846-2.diff is a refreshed patch that runs successfully through the comment group tests.

#8 @whyisjake
3 months ago

  • Keywords commit added

#9 @whyisjake
2 months ago

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

In 46335:

Comments: Avoid PHP notice in get_comment_reply_link() for null comment.

If there is no global comment, or the input comment is null, return early to prevent warnings.

Fixes #41846
Props birgire, earnjam

#11 @johnbillion
2 months ago

@birgire @whyisjake What bug does this fix? Returning early when there's no global comment silences the error which, in my eyes, seems to be a legitimate one. Calling this function in a context where there's no global comment is incorrect.

#12 @birgire
2 months ago

@johnbillion good question

At the time I think I might have looked at the validation on some of the other input arguments, that also returns null when invalid, correpsonding to the existing void part of the docs:

 * @return void|false|string Link to show comment form, if successful. False, if comments are closed.

I might have also looked into get_post() as a reference point too, for a typical WordPress practice, as it returns null on failure of finding a global post object:

https://core.trac.wordpress.org/browser/tags/5.2/src/wp-includes/post.php#L726

But I'm all in for helping devs to better debug their code.

Note: See TracTickets for help on using tickets.