comment_text filter used differently in two places in core

[ronalfy]

The 'comment_text' filter is being used differently in two different places in core.

For the life of me, I can't figure out why.

The first instance is being used as a comment is posted in /wp-includes/comment.php, passing only one argument:

$comment = apply_filters( 'comment_text', $comment );

The 2nd instance is being used when a comment is displayed in /wp-includes/comment-template.php, passing two arguments.

echo apply_filters( 'comment_text', get_comment_text( $comment_ID ), $comment );

The problem is, if someone is hooked into the 'comment_text' filter and is expecting two arguments, the 2nd one won't be set when a comment is posted.

I recommend changing the filter name 'comment_text' when the comment is being posted.

Thoughts?

[ericmann]

Nope, not a bug. Two different contexts.

In the first (/wp-includes/comment.php), the $comment variable contains just the comment text. In the second (/wp-includes/comment-template.php), the $comment variable contains the actual comment object itself (based on the return of get_comment( $comment_ID )).

So the filters are filtering the same fields. The issue then becomes the number of parameters and that, too, is not a bug.

The first comment text filter is used specifically to filter the text of the comment before things like inserting it into the database. The second is used when filtering the text of the comment before displaying it on a page - this follows the pattern of not trusting data before inserting it in the DB and also not trusting the data when pulling it back out. Using the same filter here helps us use exactly the same rules to filter the content both pre and post DB.

If the problem is the number of fields, simply make the second parameter of your function optional:

function filter_text( $comment_text, $comment = null ) {
    // Do something to the comment, possibly switching based on the presence of $comment
}
add_filter( 'comment_text', 'filter_text', 10, 2 );

If you only want to do things when the comment is inserted into the DB, just verify that null === $comment in the function above. If you only want to do things when comments are displayed, verify that $null !== $comment.

IMO this isn't a bug but a pretty powerful differentiating factor between a filter that can be used in different places to do either the same or different things.

[ronalfy]

@ericmann,

Thanks for that explanation. I'll agree that it's not a bug. Your solution is exactly what I did to get rid of any pesky errors.

[fatmedia]

Any good reason why core can't just pass those extra params as null values rather than expecting developers to know the context has mysteriously changed and account for it?

[rjeady]

A better question: why is the same filter used for both contexts? Inserting a comment into the database and rendering a comment are very different situations. I struggle to imagine why you'd want to perform the same comment text filtering twice.

I find using the same filter for both confusing and the workaround to detect which context we're in even more so.

[dd32]

IMHO: The pre-check filter should pass null and array() for $args, and the filter documentation should be updated to account for it. The documentation should explicitly point out that the comment context is unavailable during comment creation/checking which is why it's null.

Ideally they'd be differently named filters, but we can't change that now.
Historically the filter probably wasn't passed the context of the comment object, which is why it'd have made sense once upon a time.

[SergeyBiryukov]

#38314 has a patch.