Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMS5: Replace thirdparty difflib #10640

Closed
5 of 6 tasks
GuySartorelli opened this issue Jan 10, 2023 · 2 comments
Closed
5 of 6 tasks

CMS5: Replace thirdparty difflib #10640

GuySartorelli opened this issue Jan 10, 2023 · 2 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 10, 2023

As part of #10488 it has been determined that we should replace the thirdparty difflib with a maintained library which should be added via composer.

A good candidate is sebastianbergmann/diff, and I have created a proof of concept PR which does some of the work to implement this library - but take a look at #10488 (comment) for other possible alternatives.

difflib is only directly referenced in SilverStripe\View\Parsers\Diff which subclasses it - but that class is in turn referenced in a few places:

  • Various tests reference it
  • SilverStripe\VersionedAdmin\Forms\DiffField in silverstripe/versioned-admin uses it (compareHTML() only, none of the underlying methods from difflib, so should be fine)
  • SilverStripe\Versioned\DiffDifferencer uses it (again using compareHTML() only, none of the underlying methods from difflib)

Acceptance Criteria

  • The thirdparty difflib is removed from the codebase, and a suitable replacement is added via composer
  • The existing functionality which uses difflib is changed to use the replacement library
  • The result is clearly correct, even if slightly different to the output from difflib
    • e.g. it's okay if, when replacing content, the removal is shown before the addition where difflib shows the addition before the removal
  • The changelog calls out that the SilverStripe\View\Parsers\Diff class no longer has all of the methods it was inheriting from difflib
  • The methods in SilverStripe\View\Parsers\Diff which were inherited from difflib are deprecated in CMS 4 Changed\Removed methods defined directly in SilverStripe\View\Parsers\Diff are deprecated in CMS 4
    • Methods inherited from the thirdparty difflib are not called out as deprecated methods.

PRs

CMS 5

CMS 4 (deprecations)

@GuySartorelli
Copy link
Member Author

Note that the acceptance criteria for updating the changelog will be done as a part of the automation in https://github.com/silverstripeltd/product-issues/issues/627

@GuySartorelli GuySartorelli removed their assignment Feb 2, 2023
@sabina-talipova sabina-talipova self-assigned this Feb 7, 2023
@sabina-talipova
Copy link
Contributor

Third party difflib library was replaced successfully. Issue is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants