Opened 8 years ago
Closed 8 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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (19)
#3
@
8 years ago
@ambrosey it is recommended to always use braces: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style
Nice patch!
#4
@
8 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
@
8 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
@
8 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.
#8
@
8 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.
8 years ago
#11
@
8 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
@
8 years ago
@boonebgorges do you mean inside the wp_update_comment_count()
function?
$post_id = (int) $post_id;
if ( ! $post_id ) {
return false;
}
#13
@
8 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.
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
or
I tried to be consistent with the file, but it uses both styles.