Make WordPress Core

Opened 6 months ago

Last modified 2 months ago

#59580 new enhancement

Test_Diff: sync with upstream

Reported by: jrf's profile jrf Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: needs-patch
Focuses: Cc:

Description

Follow up on ticket #58298.

The Text_Diff library, as included in WP, is an external dependency, which hasn't been kept up to date with the original source - i.e. with "upstream".

The WP local version has also received some WP-only patches, typically for issues related to compatibility with new PHP versions.

It should be investigated if there are changes upstream which need to be included in the WP version of the library and if the WP version of the library contains any fixes which aren't included yet upstream and should be (and don't have an open pull request upstream).

Relevant parts from the conversation in #58298:

@SergeyBiryukov in https://core.trac.wordpress.org/ticket/58298#comment:3:

The Text_Diff library is an external dependency, however, AFAIK it is no longer externally maintained.

The pear/Text_Diff project on GitHub had a release in 2019 and appears to have recent commits, though is indeed no longer actively maintained, as it has been deprecated and superseded by Horde_Text_Diff.

We could probably consider switching to the newer package. On at least one occasion though, we encountered a bug in the upstream version, see comment:5:ticket:41526, that was patched using a different approach in [42028] / #41526. As far as I can tell, there were no further changes upstream after the commits in question, so the issue still exists there.

I have not yet checked whether a similar fix for pear/Text_Diff has the same issue.

I wonder if it may be prudent to create a repo to maintain this code as a package within the WP organisation ?

Would it be worth comparing the current code in core with the pear/Text_Diff version and checking if a switch would be possible? Otherwise, it looks like we may indeed have to continue maintaining this package for the time being, either in core or separately, to ensure compatibility with newer PHP versions.

@jrf in https://core.trac.wordpress.org/ticket/58298#comment:6:

Yes, I think it would be very good if we could take the following actions (probably in a separate ticket though):

  • Compare the current version of the package in WP Core with the upstream version of pear/Text_Diff.
  • Compare the current version of the package in WP Core with the upstream horde/Text_Diff package.

Based on the findings, discuss follow-up steps.

If we'd decide to upgrade to either the newer version of pear/Text_Diff or to horde/Text_Diff, I do still think adding at least _some_ tests to safeguard the integration would be a good thing. That way we can also safeguard that the upgrade does not cause any avoidable problems (at least for those situations which we are testing for).

@oglekler in https://core.trac.wordpress.org/ticket/58298#comment:10:

This open PR is addressing the issue we had fixed from our side: https://github.com/pear/Text_Diff/pull/8/files

Change History (3)

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


6 months ago

#2 @jorbin
6 months ago

  • Milestone changed from Awaiting Review to 6.5
  • Version trunk deleted

The changes talked about here happened in 6.3 and other versions, so I'm removing trunk from the version.

I think that figuring this out would be a good thing to do in 6.5 so I'm going to optimistically milestone it.

#3 @swissspidy
2 months ago

  • Milestone changed from 6.5 to 6.6
  • Type changed from defect (bug) to enhancement
Note: See TracTickets for help on using tickets.