WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 8 months ago

Last modified 7 months ago

#41524 closed task (blessed) (fixed)

Remove usage of each() from WP_Upgrader

Reported by: johnbillion Owned by: dd32
Milestone: 4.9 Priority: normal
Severity: major Version:
Component: Upgrade/Install Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description

Ticket split out from #40109.

WP_Upgrader::clear_destination() 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_Upgrader::clear_destination() does this.

Attachments (2)

41524.diff (1.2 KB) - added by johnbillion 11 months ago.
41524_recursive.diff (2.9 KB) - added by chris@… 9 months ago.
Recursive version of flatten_dir_list

Download all attachments as: .zip

Change History (11)

@johnbillion
11 months ago

#1 @johnbillion
11 months ago

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

This needs testing and, ideally, unit test coverage. Suggestions welcome.

#2 @johnbillion
11 months ago

  • Keywords needs-unit-tests added

#3 @johnbillion
11 months ago

Actually, I'm going to wager that this does not work, because the intention of the original code was to append items to $_files during iteration, so that they too are iterated over.

Kill me.

@chris@…
9 months ago

Recursive version of flatten_dir_list

#4 @chris@…
9 months ago

I added a recursive version of flattening the files from $wp_filesystem->dirlist(). I'm not 100% certain on rules for private or underscore naming for internal functions.

I'd add a test but I'm honestly not sure where to start because as far as I can tell there's no tests on the WP_Upgrader class.

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


9 months ago

#6 @dd32
8 months ago

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

In 41821:

Upgrades: Remove the usage of each() from WP_Upgrader for PHP 7.2 compatibility.

Props chrisvendiadvertisingcom, dd32.
Fixes #41524

#7 @dd32
8 months ago

Thanks for the patch @chris@…, I simplified it a little bit and removed the usage of passed references to simplify the code a little.

There currently isn't any unit testing of the upgrade functionalities, I've skipped including one as I'm not sure of the best method on how to do that either.

#8 @dd32
7 months ago

In 42214:

Upgrade: Fix updating plugins which include a numeric file/folder names.
The fix in [41821] caused numeric folder names to be reindexed to 0..n when in the root directory (for example, my-plugin/24/).

Props edo888.
See #41524.
Fixes #42628 for trunk.

#9 @dd32
7 months ago

In 42215:

Upgrade: Fix updating plugins which include a numeric file/folder names.
The fix in [41821] caused numeric folder names to be reindexed to 0..n when in the root directory (for example, my-plugin/24/).

Props edo888.
See #41524.
Merges [42214] to the 4.9 branch.
Fixes #42628 for 4.9.

Note: See TracTickets for help on using tickets.