WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 8 weeks ago

#41526 closed task (blessed) (fixed)

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 has-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 (5)

41526.patch (1.4 KB) - added by bor0 3 months ago.
rev1.txt (14.9 KB) - added by ocean90 2 months ago.
rev2.txt (8.7 KB) - added by ocean90 2 months ago.
41526.2.patch (3.2 KB) - added by SergeyBiryukov 8 weeks ago.
41526.3.patch (1.3 KB) - added by johnbillion 8 weeks ago.

Download all attachments as: .zip

Change History (20)

#1 @johnbillion
5 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
3 months ago

#2 @bor0
3 months 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 months ago

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

#4 @johnbillion
3 months 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 months 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 months ago

@ocean90
2 months ago

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


8 weeks ago

#7 @westonruter
8 weeks ago

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

#8 @johnbillion
8 weeks ago

In 42022:

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

See #41526

#9 @johnbillion
8 weeks ago

  • Milestone changed from 4.9 to 4.9.1

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


8 weeks ago

#11 @SergeyBiryukov
8 weeks ago

@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.

#12 @SergeyBiryukov
8 weeks ago

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.

#13 @johnbillion
8 weeks ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 4.9.1 to 4.9
  • Status changed from reopened to reviewing

#14 @johnbillion
8 weeks ago

In 42027:

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

See #41526

@johnbillion
8 weeks ago

#15 @johnbillion
8 weeks ago

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

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

Note: See TracTickets for help on using tickets.