Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#54379 closed defect (bug) (fixed)

`get_comment_ID()` can cause a notice within the `comment_text` filter

Reported by: dd32's profile dd32 Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch
Focuses: Cc:

Description (last modified by dd32)

The comment_text filter is primarily run with the context of a comment, however, during comment submission check_comment() calls the filter without any comment context, and without a global $comment object.

The result of this is that if any plugin is filtering on comment_text calls get_comment_ID() then it'll trigger the following notice:

E_NOTICE: Trying to get property 'comment_ID' of non-object in wp-includes/comment-template.php:677

Most of the comment template functions are written with the expectation that get_comment() will always return a non-false value, but get_comment_ID() seems to be the worst offender, at least among the plugins being used on the WordPress.org site.

The simplest way to trigger this is using wp-cli shell:

wp> error_reporting( E_ALL );
int(32767)

wp> ini_set( 'display_errors', true );
string(0) ""

wp> add_filter( 'comment_text', function( $text ) { return get_comment_ID(); } );
bool(true)

wp> check_comment( "author", "email", "url", "comment", "ip", "agent", "type" );

Notice: Trying to get property 'comment_ID' of non-object in wp-includes/comment-template.php on line 677

Call Stack:
.......
   25.5958   67872048  15. eval('return check_comment( "author", "email", "url", "comment", "ip", "agent", "type" );') phar://bin/wp/vendor/wp-cli/shell-command/src/WP_CLI/Shell/REPL.php:46
   25.5958   67872048  16. check_comment($author = 'author', $email = 'email', $url = 'url', $comment = 'comment', $user_ip = 'ip', $user_agent = 'agent', $comment_type = 'type') phar://bin/wp/vendor/wp-cli/shell-command/src/WP_CLI/Shell/REPL.php(46) : eval()'d code:1
   25.5960   67872048  17. apply_filters($hook_name = 'comment_text', $value = 'comment', NULL, []) wp-includes/comment.php:48
   25.5961   67872456  18. WP_Hook->apply_filters($value = 'comment', $args = [0 => 'comment', 1 => NULL, 2 => []]) wp-includes/plugin.php:189
   25.5969   67882792  19. WP_CLI\Shell\REPL->{closure:phar://bin/wp/vendor/wp-cli/shell-command/src/WP_CLI/Shell/REPL.php(46) : eval()'d code:1-1}($text = 'comment') wp-includes/class-wp-hook.php:305
   25.5969   67882792  20. get_comment_ID() phar://bin/wp/vendor/wp-cli/shell-command/src/WP_CLI/Shell/REPL.php(46) : eval()'d code:1

bool(false)

Attached is a targeted patch for get_comment_ID().

Attachments (4)

54379.diff (1.6 KB) - added by dd32 3 years ago.
54379.get_comment_author.diff (2.2 KB) - added by dd32 3 years ago.
54379.comments_open.diff (2.1 KB) - added by dd32 3 years ago.
54379.2.diff (1.6 KB) - added by dd32 3 years ago.
54379.diff + hook documentation fix. [30092] added $comment, not $comment_ID.

Download all attachments as: .zip

Change History (15)

@dd32
3 years ago

#1 @dd32
3 years ago

  • Description modified (diff)

#2 @dd32
3 years ago

Most of the comment template functions are written with the expectation that get_comment() will always return a non-false value

Other examples:

E_NOTICE: Trying to get property 'comment_status' of non-object in wp-includes/comment-template.php:1244
E_NOTICE: Trying to get property 'ping_status' of non-object in wp-includes/comment-template.php:1274

In those cases the $post object is expected to exist. See 54379.comments_open.diff. Seems to primarily happen during the jp_sitemap_cron_hook cron event, so I'm guessing it's specifically for comments that relate to post_id = 0.

E_NOTICE: Trying to get property 'user_id' of non-object in wp-includes/comment-template.php:28
E_NOTICE: Trying to get property 'comment_ID' of non-object in wp-includes/comment-template.php:48

Those are when get_comment_author() is called without a valid comment set, similar to the original issue here. See 54379.get_comment_author.diff

#3 @dd32
3 years ago

#54149 is related to the first instance here - although it wasn't included in the PR/patch to start with over there.

The other two patches are unrelated enough though.

@dd32
3 years ago

54379.diff + hook documentation fix. [30092] added $comment, not $comment_ID.

#4 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

#5 @davidbaumwald
2 years ago

  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#6 follow-up: @davidbaumwald
2 years ago

  • Keywords needs-refresh added

@dd32 After [52205], the 54379.2.diff doesn't apply cleanly.

More importantly, the inline docs there were updated to have the $comment_ID param to expect a string. Should this be corrected back to int while also casting the $comment_ID to an (int)?

Is it your preference to handle the other related notices here as well, or should those be split off into separate tickets?

This ticket was mentioned in PR #2373 on WordPress/wordpress-develop by dd32.


2 years ago
#7

  • Keywords needs-refresh removed

#8 in reply to: ↑ 6 @dd32
2 years ago

Replying to davidbaumwald:

More importantly, the inline docs there were updated to have the $comment_ID param to expect a string. Should this be corrected back to int while also casting the $comment_ID to an (int)?

While I don't agree with the whole A numeric string, for compatibility reasons. stuff, I don't see any benefit in casting to an int here, might as well just continue the status-quo of a numeric string.

Is it your preference to handle the other related notices here as well, or should those be split off into separate tickets?

I don't see the harm in fixing multiple similar things in a singular ticket. I'd just manually apply the diff if I was committing this.. but happy to refresh it for you.

Looks like 54379.comments_open.diff was handled in [52223].

https://github.com/WordPress/wordpress-develop/pull/2373 should be the remaining diffs from the attachments here.

#9 @davidbaumwald
2 years ago

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

In 52818:

Comments: Guard against potential PHP notices in get_comment_author and get_comment_ID.

In both get_comment_author and get_comment_ID, it's possible that these functions are called without a comment context. Specifically, get_comment_author can be called without passing the $comment_ID parameter, and get_comment_ID relies on the comment global if there's no $comment parameter supplied. This leads to a PHP notice of "Trying to get property of a non-object."

This change adds a check to both functions to ensure the $comment->comment_ID property is not empty before actually using its value.

Follow-up to [52223].

Props dd32.
Fixes #54379.

#11 @davidbaumwald
2 years ago

#54607 was marked as a duplicate.

Note: See TracTickets for help on using tickets.