WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37416 closed defect (bug) (fixed)

In WP_Comment_Query::fill_descendants() compute the size beforehand and not on each iteration

Reported by: ocean90 Owned by: wonderboymusic
Milestone: 4.7 Priority: low
Severity: normal Version: 4.4
Component: Comments Keywords: good-first-bug has-patch needs-testing
Focuses: performance Cc:

Attachments (3)

37416.patch (553 bytes) - added by vishalkakadiya 3 years ago.
37416_1.2.patch (554 bytes) - added by vishalkakadiya 3 years ago.
We need to go till end so needed $i <= $level_count, <= because loop is starting from 1.
37416.2.patch (537 bytes) - added by deremohan 3 years ago.
Calculating count inside "for" loop initialisation statement

Download all attachments as: .zip

Change History (12)

@vishalkakadiya
3 years ago

We need to go till end so needed $i <= $level_count, <= because loop is starting from 1.

@deremohan
3 years ago

Calculating count inside "for" loop initialisation statement

#1 @deremohan
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#2 @wonderboymusic
3 years ago

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

In 38297:

Comments: in WP_Comment_Query::fill_descendants(), compute count() in the first for expression so that it does not run on each iteration.

Props vishalkakadiya, deremohan.
Fixes #37416.

#3 follow-up: @wonderboymusic
3 years ago

In 38298:

Comments: in WP_Comment_Query::fill_descendants(), continue if there is an empty array in the loop.

See #37416, [38297].

#4 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 4.7

#5 in reply to: ↑ 3 @nacin
3 years ago

Replying to wonderboymusic:

In 38298:

Comments: in WP_Comment_Query::fill_descendants(), continue if there is an empty array in the loop.

See #37416, [38297].

@wonderboymusic, was this necessary after [38297]? Or is this just another optimization on top of that?

#6 @wonderboymusic
3 years ago

@nacin notices were generated in PHP7

#7 @ocean90
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@nacin See https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/153889644#L380.

[38298] isn't needed if the condition in [38297] wasn't changed from < to <=. Was there a reason for that? I don't think so since the index starts at 0.

#8 @jorbin
3 years ago

@ocean90 Do you want to write an update to revert the < to <= part of [38297]?

#9 @ocean90
3 years ago

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

In 38639:

Comments: Revert [38298].

Instead use the correct comparison operator which was changed in [38297].

Fixes #37416.

Note: See TracTickets for help on using tickets.