Make WordPress Core

Opened 3 years ago

Last modified 6 days ago

#24913 reopened defect (bug)

comment_text filter used differently in two places in core

Reported by: ronalfy Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.6
Component: Comments Keywords: dev-feedback
Focuses: Cc:


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.


Change History (4)

#1 @ericmann
3 years ago

  • Keywords 2nd-opinion removed

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.

#2 @ronalfy
3 years ago

  • Resolution set to invalid
  • Status changed from new to closed


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.

#3 @DrewAPicture
3 years ago

  • Milestone Awaiting Review deleted

#4 @fatmedia
6 days ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

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?

Note: See TracTickets for help on using tickets.