Make WordPress Core

Opened 11 years ago

Closed 8 years ago

#24913 closed defect (bug) (duplicate)

comment_text filter used differently in two places in core

Reported by: ronalfy's profile ronalfy Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.6
Component: Comments Keywords:
Focuses: Cc:

Description

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?

Change History (7)

#1 @ericmann
11 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
11 years ago

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

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

#3 @DrewAPicture
11 years ago

  • Milestone Awaiting Review deleted

#4 @fatmedia
8 years 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?

#5 @rjeady
8 years ago

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.

#6 @dd32
8 years ago

  • Keywords needs-patch good-first-bug added; dev-feedback removed
  • Milestone set to Awaiting Review

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.

#7 @SergeyBiryukov
8 years ago

  • Keywords needs-patch good-first-bug removed
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

#38314 has a patch.

Note: See TracTickets for help on using tickets.