Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#41525 closed task (blessed) (fixed)

Remove usage of each() from WP_PHPUnit_Util_Getopt

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-testing
Focuses: Cc:

Description

Ticket split out from #40109.

WP_PHPUnit_Util_Getopt::__construct() uses each(), which is deprecated in PHP 7.2 and therefore should be removed.

The each() function exhibits two pieces of behaviour which mean it can't simply be replaced with foreach():

  1. After each() has executed, the array cursor will be left on the next element of the array, or past the last element if it hits the end of the array. This means you can break out of an each loop, and continue the iteration at a later point. Text_Diff_Engine_native::_diag() does just this.
  2. With the while ( each( $array ) ) pattern, it's possible to alter the elements in the array during iteration.

WP_PHPUnit_Util_Getopt::__construct() does just this, but in a very non-obvious manner. $argv is passed into PHPUnit_Util_Getopt::parseLongOption() as a reference (ref), whereupon its array cursor is moved on (ref) under some condition.

This mess needs to be fixed.

Attachments (1)

41525.patch (652 bytes) - added by ayeshrajans 8 years ago.

Download all attachments as: .zip

Change History (8)

#1 @ocean90
8 years ago

#41530 was marked as a duplicate.

#2 @ayeshrajans
8 years ago

The \WP_PHPUnit_Util_Getopt class is hard to test because it modifies some static variables. However, I took a try to get rid of the each() call.

The $i variable was not used. We still keep the while() pattern because it needs to be modified. However, instead of using each() to fetch and advance the array iterator, this patch proposes to do it in separate calls. The immediately following next() call advances the array pointer, making the while() loop finish.

@ayeshrajans
8 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


8 years ago

#4 @johnbillion
8 years ago

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

#5 @johnbillion
7 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#6 @johnbillion
7 years ago

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

In 41636:

Build/Test tools: Update the WP_PHPUnit_Util_Getopt class for PHP 7.2 compatibility.

This removes usage of each() which is deprecated in PHP 7.2.

Props ayeshrajans

See #40109
Fixes #41525

#7 @johnbillion
7 years ago

In 41637:

Build/Test tools: Update the WP_PHPUnit_Util_Getopt class for PHP 7.2 compatibility.

This removes usage of each() which is deprecated in PHP 7.2.

See #40109, #41525

Merges [41636] to the 4.8 branch.

Note: See TracTickets for help on using tickets.