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.
@ambrosey So it looks like on line 1951 you are just missing a space before the bracket.

Version 0, edited 4 years ago by rachelbaker (next)

#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.