Remove usage of each() from Text_Diff_Engine_native

[johnbillion]

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.

[johnbillion]

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]

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.

[johnbillion]

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

[ocean90]

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.

[westonruter]

@johnbillion It looks like we'll have to revert [41633] for 4.9 if the issue in comment #5 isn't resolved.

[johnbillion]

In 42022:

External Libraries: Revert [41633]. This causes warnings when editing and viewing certain revisions.

See #41526

[SergeyBiryukov]

@johnbillion 41526.2.patch​ is an alternative approach that should be functionally equivalent to the existing code and does not produce any warnings following the steps from comment:5.

I've also confirmed there are no visual changes before and after the patch following the same steps.

[SergeyBiryukov]

For reference, here are the upstream commits that [41633] was based upon:
​https://github.com/horde/Text_Diff/commit/65f92a945f3745be2953d2bcc24735e860cf70a7
​https://github.com/horde/Text_Diff/commit/bd9b0e05ad38ad32d55cf7188617ec3b0d906088

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.

Looks like it does have tests: ​https://github.com/horde/Text_Diff/tree/master/test/Horde/Text/Diff. If someone can write a test using the data from comment:5, and/or figure out why exactly the foreach() version fails, might be worth reporting this upstream.

[johnbillion]

In 42027:

External Libraries: Remove usage of text strings in assert() in the Text_Diff_Engine_native class.

See #41526

[johnbillion]

In 42028:

External Libraries: Remove usage of each() from the Text_Diff_Engine_native class.

This removes deprecated notices in PHP 7.2 but takes a different approach to the upstream class from Horde, which appears to be buggy.

Props SergeyBiryukov
Fixes #41526