WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 2 weeks ago

#41526 reopened task (blessed)

Remove usage of each() from Text_Diff_Engine_native

Reported by: johnbillion Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

Ticket split out from #40109.

Text_Diff_Engine_native::_diag() 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.

Text_Diff_Engine_native::_diag() iterates over $matches with each() until the condition which causes the break clause is met, upon which the second call to each() continues iteration from the same point in the array. Lovely.

The Text_Diff library is a complex, third-party script with no test coverage, so changes here need to be made carefully. Unit tests probably could be introduced to demonstrate no regressions when each() is replaced.

Attachments (3)

41526.patch (1.4 KB) - added by bor0 4 weeks ago.
rev1.txt (14.9 KB) - added by ocean90 2 weeks ago.
rev2.txt (8.7 KB) - added by ocean90 2 weeks ago.

Download all attachments as: .zip

Change History (8)

#1 @johnbillion
3 months ago

Also worth pointing out that it's not worth trying to push this fix back upstream because the library is now part of Horde which hides its sources behind a login.

@bor0
4 weeks ago

#2 @bor0
4 weeks ago

  • Keywords has-patch added; needs-patch removed

To implement this patch, I downloaded the library from https://dev.horde.org/ and ran a diff comparison for that file and specifically that function.

#3 @johnbillion
3 weeks ago

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

#4 @johnbillion
3 weeks ago

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

In 41633:

External Libraries: Update the Text_Diff_Engine_native class for PHP 7.2 compatibility.

This removes usage of each() and usage of text strings passed to assert().

Props bor0

Fixes #41526

#5 @ocean90
2 weeks ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Since [41633] I'm seeing a few warnings like

Warning: assert(): assert($y <= $this->seq[$k]) failed in wp-includes/Text/Diff/Engine/native.php on line 199

Warning: assert(): assert($ypos != $this->seq[$end]) failed in wp-includes/Text/Diff/Engine/native.php on line 239

I'm attaching rev1.txt and rev2.txt.

  • Create a new post with rev1.txt
  • Update content of the post with rev2.txt.
  • Go to the revisions screen where you should notice the warnings. I also got them once during updating the post.

@ocean90
2 weeks ago

@ocean90
2 weeks ago

Note: See TracTickets for help on using tickets.