Make WordPress Core

Opened 10 years ago

Last modified 3 months ago

#36409 assigned defect (bug)

Comments number is wrong

Reported by: sidati's profile sidati Owned by: pbearne's profile pbearne
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests changes-requested
Focuses: template Cc:

Description

Hi,
the comment number return the number of approved comments, this means all approved comments even those are replies to unapproved comments, for example if a user post comment and people replied to it, then the admin decide to hide this parent comment by disapproving it, the comment number will decrease only by one and keeping counting the hidden replies

So there are two option to fix that :

  1. Hold/Disapprove all children when disapproving the parent.
  2. Recalculate the comment number and excluding comments has an unapproved parent.

Regards,
Sidati

Attachments (3)

36409.patch (1.1 KB) - added by sidati 10 years ago.
36409-2.patch (1.4 KB) - added by sidati 10 years ago.
36409-3.patch (1.4 KB) - added by sidati 10 years ago.

Download all attachments as: .zip

Change History (28)

@sidati
10 years ago

#1 @sidati
10 years ago

  • Keywords has-patch added

I prefer the 2nd option even it's not so good for performance, maybe someone has better idea :)
https://core.trac.wordpress.org/attachment/ticket/36409/36409.patch

Version 0, edited 10 years ago by sidati (next)

#2 @sidati
10 years ago

  • Keywords has-patch removed

#3 @sidati
10 years ago

  • Keywords has-patch added

Working Patch and more optimized

@sidati
10 years ago

#4 @boonebgorges
10 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

Hi @sidati - Thanks for the patch. I agree that this is unexpected and incorrect behavior.

Your basic idea for fixing it seems right to me. A couple comments on the patch:

  • Could you generate it from the root of your WP installation? It makes it a bit easier to apply and test.
  • Your $depth logic reads backward to me. I mean, I can see that the patch works, but I would've counted the depth up instead of down. Maybe our minds work differently ;) Anyway, I wonder if the algorithm would be more transparent (and less subject to bugs related to 'thread_comment_depth' if it looked something like this (pseudocode):
$bad_parents = $wpdb->get_col( "SELECT comment_ID from $wpdb->comments WHERE comment_approved != 1" );
$_bad_parents = array();

do {
    $_bad_parents = $wpdb->get_col( "SELECT comment_ID from $wpdb->comments WHERE comment_parent IN (" . implode( ',', array_map( 'intval', $bad_parents ) ) . ")";
    $bad_parents += $_bad_parents;
} while ( $_bad_parents );

This separates the initial comment_approved query for clarity, and it ensures that we keep walking down the tree as long as bad comments are found.

We will also need unit tests to cover the problem.

@sidati
10 years ago

#5 @sidati
10 years ago

@boonebgorges;
I like your approach but if you realize the loop is infinite :), so i add a third variable to hold only the found children IDs to be used in the next loop.

You can test it now : 36409-3.patch

#6 in reply to: ↑ description @ambrosiawt
10 years ago

For what it's worth, this looks very similar to #18603. I sort of hope it fixes that one as well. :)

Art

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#7 @chriscct7
10 years ago

  • Version trunk deleted

This ticket was mentioned in Slack in #core-comments by sidati. View the logs.


9 years ago

#9 @sidati
9 years ago

I'll create a plugin on github to solve this issue and post it here with unit-tests

#10 @pbearne
18 months ago

@sidati did you ever create the unit tests?

This ticket was mentioned in PR #6431 on WordPress/wordpress-develop by @pbearne.


18 months ago
#11

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

This update revises the logic used in the wp_update_comment_count_now function. The change ensures that unapproved comments and their children are not included when calculating the comment count for a post. This method involves creating a list of 'bad parents', which represents comments that are not approved, and their associated child comments.

#12 @pbearne
18 months ago

I have refreshed the patched

#13 @pbearne
18 months ago

  • Owner set to pbearne
  • Status changed from new to assigned

#14 @sidati
18 months ago

Hey @pbearne, It’s been years and I don’t remember creating any test.

#15 @pbearne
9 months ago

  • Milestone set to 6.8

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

#17 @audrasjb
8 months ago

  • Milestone changed from 6.8 to 6.9

As per today's bug scrub, I'm moving it to 6.9 for further review/discussion.

This ticket was mentioned in Slack in #core-performance by adamsilverstein. View the logs.


5 months ago

#19 @adamsilverstein
5 months ago

  • Focuses performance removed

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


5 months ago

#21 @jeannetteunifreiburg
5 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6431

Environment

  • WordPress: 6.8.1
  • PHP: 8.2.27
  • Server: nginx/1.26.1
  • Database: mysqli (Server: 8.0.35 / Client: mysqlnd 8.2.27)
  • Browser: Firefox 138.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ❌ Issue NOT resolved with patch. Comment count is still wrong.
  2. Raw file downloaded from git has syntactical errors. When I fix that (missing semicolon), the bug is still there.
  3. When I try to run the PR in playground, playground crashes (MacOS, both Firefox and Safari)

Additional Notes

  • None

Supplemental Artifacts

None

#22 follow-up: @SirLouen
5 months ago

  • Keywords changes-requested added

@pbearne it seems that apart from the issue with the semicolon, the patch is not passing.
Can you give it a go when you find some time?

#23 in reply to: ↑ 22 @pbearne
5 months ago

Replying to SirLouen:

@pbearne it seems that apart from the issue with the semicolon, the patch is not passing.
Can you give it a go when you find some time?

Updated

This ticket was mentioned in PR #9277 on WordPress/wordpress-develop by @hbhalodia.


3 months ago
#24

Trac ticket: https://core.trac.wordpress.org/ticket/36409

## Fixes

  1. Get all the comments related to post and created the lookup based on the comment ID.
  2. Loop through each comment and check for its child<-->parent and add only if parent comment is approved.

#25 @hbhalodia
3 months ago

Hi Team, I have raised the new PR which uses PHP logic to check for the parent and child comment and update the comment count for the comments that are approved and child comments whose parents are approved.

PR - https://github.com/WordPress/wordpress-develop/pull/9277

Have checked the above patch and it was not wokring as expected, hence worked on the same and created a new PR for it.

Thank You,

Last edited 3 months ago by hbhalodia (previous) (diff)
Note: See TracTickets for help on using tickets.