Allow filtering of comment count

[peterwilsoncc]

Adding a filter to wp_update_comment_count_now would be handy. Use cases:

• sites collect comment types but don't display them
• not comment comments such as those used by edd and woo commerce.

Related #12668

[rachelbaker]

@peterwilsoncc Sounds reasonable. Want to work up a patch?

[peterwilsoncc]

@swissspidy that was my first thought too, but figured it might be best to avoid the DB call in the function if possible, see 35060.3.diff​.

Unsure if the filter is documented properly with the null constant.

No tests exist for wp_update_comment_count_now, where are these best placed, does it need to be a seperate ticket?

[rachelbaker]

@peterwilsoncc A better name for the filter would probably be pre_wp_update_comment_count_now since it happens before the DB call.

The unit tests should go in a new file: /tests/phpunit/tests/comment/wpUpdateCommentCountNow.php

[rachelbaker]

@swissspidy Curious why you are using null as the default value of the wp_update_comment_count_now filter? Is there any reason we should not just pass in the $new comment count?

[peterwilsoncc]

Replying to rachelbaker:

@swissspidy Curious why you are using null as the default value of the wp_update_comment_count_now filter? Is there any reason we should not just pass in the $new comment count?

That was my suggestion, the intention is to avoid the extra database lookup if the filter returns a valid value. The query is uncached and comment_approved is not indexed.

[swissspidy]

@rachelbaker So..what do you think about this approach with null (or maybe false) as the default?

[rachelbaker]

@swissspidy Personally, I don't like the idea of using a filter to bail out of the query. I couldn't find another example of this within the codebase. Would an action be a better fit here?

[peterwilsoncc]

For what it's worth, the basis was the 'pre_option_' . $option filter in get_option(). I can't think where else something similar happens, though.

[boonebgorges]

Replying to peterwilsoncc:

For what it's worth, the basis was the 'pre_option_' . $option filter in get_option(). I can't think where else something similar happens, though.

$ ack "apply_filters\([^\,]+, null" src/ suggests a few places:

• get_option()
• get_site_by_path()
• get_avatar()
• get_metadata()
• get_network_by_path()

The pattern is: if a pre_ filter allows developers to avoid expensive queries, then it's justified. I think it's reasonable in this case.

[swissspidy]

35060.5.diff​ simplifies the patch and also fixes docblock formatting. Adds pre_ to the filter name.

[rachelbaker]

@swissspidy looks good

[swissspidy]

In 36318:

Comments: Add a new pre_wp_update_comment_count_now filter.

This allows filtering a post's comment count before it is queried and updated in the database.

Props peterwilsoncc for initial patch.
Fixes #35060.

[jorbin]

In 37445:

Correct usage of Covers for Tests_Update_Comment_Count_Now

When @covers is used with a plain alphanumeric string, PHPUnit assumes that it is covering a class. If there is no class, it fails and exits rather than continuing to generate coverage. To cover a global function, the name must start with ::. See ​https://phpunit.de/manual/5.3/en/appendixes.annotations.html#appendixes.annotations.covers

See #35060, #36867

[jorbin]

In 37447:

Correct usage of Covers for Tests_Update_Comment_Count_Now

When @covers is used with a plain alphanumeric string, PHPUnit assumes that it is covering a class. If there is no class, it fails and exits rather than continuing to generate coverage. To cover a global function, the name must start with ::. See ​https://phpunit.de/manual/5.3/en/appendixes.annotations.html#appendixes.annotations.covers

[37445] for trunk

See #35060, #36867