Make WordPress Core

Opened 21 months ago

Last modified 20 months ago

#56499 new defect (bug)

count() used in the loop condition

Reported by: krunal265's profile krunal265 Owned by:
Milestone: Awaiting Review Priority: lowest
Severity: normal Version:
Component: Comments Keywords: needs-testing has-patch
Focuses: administration, coding-standards Cc:

Description

The use of count() inside a loop condition is not allowed; assign the return value to a variable and use the variable in the loop condition instead.

Attachments (2)

56499.patch (614 bytes) - added by krunal265 21 months ago.
patch added.
56499.diff (1.1 KB) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (12)

@krunal265
21 months ago

patch added.

#1 @johnbillion
21 months ago

  • Keywords needs-testing added
  • Severity changed from major to normal

Thanks for the patch @krunal265 . Have you tested this change? The $comments array gets populated inside the while which means this change will break this functionality because the count is no longer updated with each iteration.

#2 @SergeyBiryukov
21 months ago

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Do you get any PHPCS warnings from this file? When running WPCS checks for WordPress core with the default phpcs.xml.dist file, wp-admin/includes/dashboard.php appears to pass all the checks successfully in my testing. Could you clarify how you run the PHPCS checks?

As noted above, the $comments array is changed inside the loop, so the value needs to be recalculated on each iteration, and the patch may not work as expected. That said, I agree that the condition could probably be simplified.

#3 @desrosj
21 months ago

  • Keywords reporter-feedback added

#4 follow-up: @krunal265
21 months ago

@johnbillion, @SergeyBiryukov Thank you for replying and looking into this ticket.
Yes, I have tested this change and it's working fine. Every new comment is displayed on the dashboard after changing this.

#5 @krunal265
21 months ago

Hello @SergeyBiryukov, @johnbillion
Any further updates regarding this issue?

#6 in reply to: ↑ 4 @SergeyBiryukov
20 months ago

  • Component changed from Administration to Comments
  • Focuses administration added
  • Milestone changed from Awaiting Review to 6.1

Replying to krunal265:

Yes, I have tested this change and it's working fine. Every new comment is displayed on the dashboard after changing this.

Thanks! I think an answer to the questions from comment:2 would be helpful to move the ticket forward, as the file passes all WPCS checks for core and it's not quite clear to me what is the exact issue we're trying to fix here:

Do you get any PHPCS warnings from this file? When running WPCS checks for WordPress core with the default phpcs.xml.dist file, wp-admin/includes/dashboard.php appears to pass all the checks successfully in my testing. Could you clarify how you run the PHPCS checks?

The patch as is does not look correct to me, as $comments is set to an empty array at the beginning of the function, so count( $comments ) would always be zero at that point, and the condition becomes redundant, though it looks like the function still works due to the break statements inside the loop.

If we need to remove the count() call from the loop condition here, we could change the loop to do ... while instead, see 56499.diff. I'm not 100% sure this change is necessary, but it might be a readability improvement, so I'm moving this to 6.1 for further discussion.

#7 @audrasjb
20 months ago

  • Milestone changed from 6.1 to Future Release

As the patch and the issue itself are still being discussed, and since beta 1 is in a few days, let's move this ticket to Future Release milestone.

@krunal265 there is still some questions to answer about the potential issue underlined in this ticket, that's why I'm moving it to Future Release. We can still move it to the next major (6.2) if the issue is clarified :)

#8 @krunal265
20 months ago

@audrasjb, We can use do...while loop in this issue instead of while loop. From this change, coding standards and readability will improve. Because the use of count() inside a loop condition is not allowed; assign the return value to a variable and use the variable in the loop condition instead.

#9 @johnbillion
20 months ago

  • Keywords needs-patch added; has-patch reporter-feedback removed
  • Milestone changed from Future Release to Awaiting Review
  • Priority changed from normal to lowest
  • Version 6.0 deleted

#10 @SergeyBiryukov
20 months ago

  • Keywords has-patch added; needs-patch removed

I believe 56499.diff addresses the request here.

Note: See TracTickets for help on using tickets.