WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34977 closed enhancement (fixed)

When updating a Comment do not call `wp_update_comment_count` if there is no related Post

Reported by: rachelbaker Owned by: rachelbaker
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Comments Keywords: good-first-bug has-patch
Focuses: Cc:

Description

As a follow-up to #34954:
In wp_update_comment() we are calling wp_update_comment_count(), but this is unnecessary if the comment_post_ID is 0.

Attachments (4)

34977.patch (919 bytes) - added by ambrosey 4 years ago.
34977-refresh.patch (916 bytes) - added by juanfra 4 years ago.
Patch with space.
34977-in-function.diff (1.1 KB) - added by juanfra 4 years ago.
Now checking inside the function.
34977.2.patch (503 bytes) - added by rachelbaker 4 years ago.
Simplified logic from @juanfra's patch

Download all attachments as: .zip

Change History (19)

#1 @rachelbaker
4 years ago

  • Keywords good-first-bug added

@ambrosey
4 years ago

#2 @ambrosey
4 years ago

  • Keywords has-patch added; needs-patch removed

I tried to patch this but it was unclear to me due to other inconsistencies in the specific file, whether it's best practice to put

if ( $variable ){
    do_this();
}

or

if ( $variable )
    do_this(); 

I tried to be consistent with the file, but it uses both styles.

#4 @ambrosey
4 years ago

@juanfra would it therefore be reasonable to change this patch to change the rest of the file to braces? Or would that be best served on a separate ticket?

#5 @juanfra
4 years ago

@ambrosey I'm not sure, let's @rachelbaker respond to that. I can help out with adding braces if the answer is yes.

#6 @rachelbaker
4 years ago

@juanfra @ambrosey Great questions. With newly written lines we do want to follow the WordPress Coding Standards.
To keep an accurate history and reduce code churn we don't reformat entire files for style reasons. Brick by brick, we will get there eventually :)

@ambrosey So it looks like on line 1951 you are just missing a space before the bracket.

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

#7 @rachelbaker
4 years ago

  • Keywords needs-refresh added

@juanfra
4 years ago

Patch with space.

#8 @juanfra
4 years ago

@rachelbaker Great, so we'll keep on tracing new tickets for that. And keep improving one line at a time.

Kudos @ambrosey :)

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


4 years ago

#10 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Status changed from new to reviewing

#11 @boonebgorges
4 years ago

  • Keywords needs-patch added; has-patch needs-refresh removed
  • Owner changed from boonebgorges to rachelbaker

As suggested by @rachelbaker, it probably makes more sense to put the ! $post_id check into wp_update_comment_count() itself. wp_update_comment_count_now() already bails when the $post_id is empty, and there's no reason why we'd ever want to add 0 to the deferred-comment-count array.

#12 @juanfra
4 years ago

@boonebgorges do you mean inside the wp_update_comment_count() function?


$post_id = (int) $post_id;
if ( ! $post_id ) {
        return false;
}

#13 @rachelbaker
4 years ago

As I dug more into this I found that this bug had a slightly larger scope. Currently only wp_delete_comment() does check if the comment_post_ID is valid (as in returns a WP_Post object from get_post()) before calling wp_update_comment_count(). Instead of adding a bunch of conditional checks before calling that function, it would be cleaner to do the check in the wp_update_comment_count() function itself, and return early if there is no post.

We will need to be careful when doing this because wp_defer_comment_counting() actually intentionally passes null for the $post_id param.

@juanfra
4 years ago

Now checking inside the function.

#14 @swissspidy
4 years ago

  • Keywords has-patch added; needs-patch removed

@rachelbaker
4 years ago

Simplified logic from @juanfra's patch

#15 @rachelbaker
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 36115:

Comments: Return early from wp_update_comment_count() if there is not a valid post.

Props ambrosey, juanfra.
Fixes #34977

Note: See TracTickets for help on using tickets.