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 | 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)
#2
@
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.
#4
@
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
@
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
@
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.
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 ofget_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:
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.