DiffEngine is a fork of a library from 17 years ago. It is obviously stable to some extent, but it's code is extremely non-Drupal and it only ever gets the bare minimum attention to port. It seems likely that eventually we're going to hit bigger issues with PHP version compatibility.
Every so often it fails a new code quality check. The most recent example of it showing up in an issue is #3274474: Fix 'Access to an undefined property' PHPStan L0 errors. It also is not PHP 8.2 compatible and triggers PHPStan L0 errors.
Deprecate DiffEngine and replace it with https://github.com/sebastianbergmann/diff
1. Decide which one (if any) to use
1a. Dependency evaluation
2. Patch.
1. Fix the 8.2 compatibility (possibly by adding phpcs-ignore/phpstan-ignore to the entire file so we don't have to rewrite the entire file to do so), then deprecate it in 10.1.x for 11.x for one of the alternatives
2. Fix the 8.2 compatibility and deprecate it in 9.5.x for 11.x
3. Don't fix the 8.2 compatiblity, and deprecate it in 9.5.x for removal in 10.0.x, but with a patch ready against diff module to switch over.
4. Don't fix the 8.2 compatibility and deprecate it in 9.5.x for 11.x (basically diff module would have to switch over to get PHP 8.2 compatibility but without a hard deadline).
This package is already in use indirectly on Drupal test environments as it is a dependency of PHPUnit, used to report diff of actuals vs expectations. It has over 450 million downloads (Feb 2023) total, over 300k downloads daily.
Maintainership: Maintained by Sebastian Bergmann and the PHPUnit community.
Security policy: https://github.com/sebastianbergmann/diff/blob/main/SECURITY.md
Release cycle: No schedule, it appears to be kept in synch with PHP evolution as needed by PHPUnit.
Other dependencies: None.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !2543
Comments
Comment #2
mondrakeThere's also sebastianbergmann/diff which is already a dependency of PHPUnit we're using in dev.
Disclaimer: I know nothing about the scope of usage here.
DiffEngine is making use of dynamic properties so it is in the blocker path for PHP 8.2 compatibility.
Comment #3
mondrakeComment #4
catchComment #5
longwaveI think this has been discussed before at least once, IIRC the issue is that if we are to retain BC then the interface we provide is very tied to the existing implementation. Alternatively we could just deprecate our diff component and reimplement where it is used - possibly only the config UI and some tests?
#1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php is related but can probably be closed as outdated now.
Comment #6
mondrakeI'm working on this
Comment #7
catchThat sounds like a good idea - means just a service + class deprecation and then providing the new ones.
Comment #8
larowlanIt looks like diff module in contrib uses it https://git.drupalcode.org/project/diff/-/blob/8.x-1.x/src/DiffEntityCom... - 41k installs of the 8.x-1.x version - so we need to be mindful of that
Comment #9
catchAdded some possible deprecation scenarios for the old service to the issue summary.
Comment #10
mondrakeTurns out we cannot deprecate easily Diff per se, but we can deprecate DiffEngine, and several methods of the component that are unused in core.
We can replace DiffEngine with sebastianbergmann/diff with a BC layer to transform its results into the
DiffOp*value objects, that are kinda Drupal specific, and quite spread around, so not easy to replace.Comment #11
mondrakeHere's a patch, still quite rough around the edges, but illustrates the idea.
Comment #12
mondrakeComment #14
mondrakeComment #15
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #16
mondrakeRebased.
Comment #17
spokjeUnhappy PHPStan :/
Comment #18
longwaveThe proposed approach looks pretty neat while keeping us within the constraints of the interface to the existing Diff component.
Adding some tags for things that we still need; we also should add an explicit dependency on
sebastian/diffto the core composer.json and also the component's composer.json.Comment #19
mondrakeAdded a draft CR and reflected in the deprecation messages, fixed PHPStan errors.
Comment #20
mondrakeComment #21
mondrakeUpdated IS and issue title
Comment #22
andypostdrupal:10.0.0needs update to 10.1.0Comment #23
mondrakeComment #24
longwaveA comment that I think can be removed now, plus we also need to add the dependency to core/lib/Drupal/Component/Diff/composer.json.
Comment #25
mondrakeThanks for the review @longwave, commented inline.
Comment #26
smustgrave commentedRemoved change record as one was added.
This was a near perfect think I think. Issue summary was clear with problem, steps, and tasks.
Reviewing the PR everything looks good, not sure what more can be looked at.
Comment #27
longwaveThe change record is only a placeholder and has no detail, also we need a dependency evaluation.
Comment #28
mondrakeFleshed out the CR.
Comment #29
mondrakeAdded dependency evaluation
Comment #30
mondrakeRebased.
Comment #31
catchThis looks good to me, we might want to look at the Diff component itself later, but no need to do that to make the switch here and the bc layer is tidy.
Since I opened this, probably better if another committer commits it, or at least will give it a few days if no-one else does.
Comment #32
longwaveCommitted and pushed ce28a42738 to 10.1.x. Thanks!
Comment #35
mondrakePublished the CR
Comment #37
alexpottUnfortunately this change has resulted in a regression in the update_helper module (and also the config_update module) see #3386482: DiffOpOutputBuilder does not correctly match the 10.0.x implementation and produces unexpected output.
Comment #38
alexpottTurns out we need one more fix for update_helper... #3386707: DiffOpOutputBuilder does not correctly match the 10.0.x implementation and produces unexpected output (part 2)
Comment #39
mstrelan commentedThis has also introduced a regression in the diff contrib module when content in revisions have different line endings.
#3389010: Old content with different line endings can break diff entirely
#3389715: Diffs with different line endings leads to Invalid $mode 3 specified