WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#35060 closed feature request (fixed)

Allow filtering of comment count

Reported by: peterwilsoncc Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.5
Component: Comments Keywords: has-patch has-unit-tests commit
Focuses: Cc:
PR Number:

Description

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

Attachments (5)

35060.diff (879 bytes) - added by swissspidy 4 years ago.
35060.2.diff (1.1 KB) - added by peterwilsoncc 4 years ago.
35060.3.diff (1.1 KB) - added by peterwilsoncc 4 years ago.
35060.4.diff (3.0 KB) - added by swissspidy 4 years ago.
35060.5.diff (3.0 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (23)

#1 @rachelbaker
4 years ago

  • Milestone changed from Awaiting Review to Future Release

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

#2 @rachelbaker
4 years ago

  • Type changed from defect (bug) to feature request

@swissspidy
4 years ago

#3 @swissspidy
4 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.5

#4 @peterwilsoncc
4 years ago

  • Keywords needs-unit-tests added

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

Last edited 4 years ago by peterwilsoncc (previous) (diff)

#5 @rachelbaker
4 years ago

@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

@swissspidy
4 years ago

#6 @swissspidy
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#7 follow-up: @rachelbaker
4 years ago

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

#8 in reply to: ↑ 7 @peterwilsoncc
4 years ago

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.

#9 @swissspidy
4 years ago

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

#10 @rachelbaker
4 years ago

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

#11 @rachelbaker
4 years ago

  • Keywords 2nd-opinion added

#12 follow-up: @peterwilsoncc
4 years ago

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.

#13 in reply to: ↑ 12 @boonebgorges
4 years ago

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
4 years ago

#14 @swissspidy
4 years ago

  • Keywords commit added; 2nd-opinion removed

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

#15 @rachelbaker
4 years ago

@swissspidy looks good

#16 @swissspidy
4 years ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

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.

#17 @jorbin
3 years ago

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

#18 @jorbin
3 years ago

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

Note: See TracTickets for help on using tickets.