WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#30412 closed defect (bug) (fixed)

get_approved_comments() behaviour change with post_id=0

Reported by: dd32 Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

In [30098] get_approved_comments() was made a wrapper of WP_Comment_Query, this has had a slight backwards compat change.

Previously, $post_id was a required parameter, and if passed a falsey value would simply return an empty array (as it'd query for post_id = 0). After the change, it now queries for comments on ALL posts, this can very easily exhaust system resources if someone was calling the function incorrectly.

Attached is a patch for review, It simply makes $post_id a required parameter, and if it evaluates false the function returns an empty array.

Attachments (2)

30412.diff (1.2 KB) - added by dd32 6 years ago.
30412.patch (1.9 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (7)

@dd32
6 years ago

#1 @dd32
6 years ago

  • Component changed from General to Comments
  • Version set to trunk

@boonebgorges
6 years ago

#2 @boonebgorges
6 years ago

Thanks for the patch, dd32, and sorry for breaking stuff :) I wanted to verify that returning all comments when passing post_id=0 to WP_Comment_Query::query() was the expected behavior in 4.0 and not a regression (it was, and it's not). So the special case in get_approved_comments() looks like the right fix to me. 30412.patch contains unit tests for passing 0 to get_approved_comments() as well as WP_Comment_Query.

I'm posting this here rather than committing it straight away because I wanted to ask you about making $post_id required. In a quick search on Github, I couldn't find any instances of get_approved_comments() being called without a parameter, but it could be happening somewhere, so I don't see a reason why we should throw a notice in that case. At least, it's not necessary to fix the bug reported in this ticket.

#3 @dd32
6 years ago

I couldn't find any instances of get_approved_comments() being called without a parameter, but it could be happening somewhere, so I don't see a reason why we should throw a notice in that case.

Previously the parameter was required & used within the SQL, since the function will now return early if it's not specified, it makes sense to restore that behaviour.

#4 @boonebgorges
6 years ago

Previously the parameter was required

*headdesk*

#5 @boonebgorges
6 years ago

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

In 30402:

Return an empty array from get_approved_comments() when $post_id is empty.

This behavior was broken when moving the internals to WP_Comment_Query in
[30098]. As a result, get_approved_comments( 0 ) was fetching *all* approved
comments, causing performance issues.

Props dd32.
Fixes #30412.

Note: See TracTickets for help on using tickets.