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

[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().

[dd32]

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​

[dd32]

#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]

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

[davidbaumwald]

@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?

Trac ticket: https://core.trac.wordpress.org/ticket/54379

[dd32]

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.

[davidbaumwald]

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.

Committed to core in https://core.trac.wordpress.org/changeset/52818

[davidbaumwald]

#54607 was marked as a duplicate.