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

Remove Text::Diff::HTML from Kernel/cpan-lib #2610

Closed
bschmalhofer opened this issue Oct 27, 2023 · 4 comments
Closed

Remove Text::Diff::HTML from Kernel/cpan-lib #2610

bschmalhofer opened this issue Oct 27, 2023 · 4 comments
Assignees
Labels
tidying Tidying of the code
Milestone

Comments

@bschmalhofer
Copy link
Contributor

Text::Diff::HTMLis loaded only in Kernel::System::Diff but not actually used there. Let's get rid of it.

@bschmalhofer bschmalhofer self-assigned this Oct 27, 2023
@bschmalhofer bschmalhofer added the tidying Tidying of the code label Oct 27, 2023
@bschmalhofer bschmalhofer added this to the OTOBO 11.0 milestone Oct 27, 2023
@bschmalhofer
Copy link
Contributor Author

Actually Text::Diff::HTML is used in Kernel::System::Diff as a styler that is loaded dynamically. But that style is set for the plain diff, which does not make sense at all. This is not critical at all, as Kernel::System::Diffis unused anyways. So let's go ahead with the removal.

bschmalhofer added a commit that referenced this issue Oct 27, 2023
of the result from Kernel::System::Diff::Compare(). This means
that Text::Diff::HTML is no longer needed. HTML diffs are
generated with Text::Diff::FormattedHTML.
bschmalhofer added a commit that referenced this issue Oct 27, 2023
Issue #2610: do not return HTML in the Plain attribute
@bschmalhofer
Copy link
Contributor Author

Done. Diff.t looks fine. Closing this issue.

@bschmalhofer
Copy link
Contributor Author

See also #1229

@stefanhaerter
Copy link
Contributor

It was discussed wether Text::Diff can also be removed. There is one occurrence in core:

# do not allow to read file with including .. path (security related)
$LocalFile =~ s/\.\.//g;
if ( !$File ) {

elsif ( !-e $LocalFile ) {

elsif ( -e $LocalFile ) {
my $Content = $MainObject->FileRead(
Location => $LocalFile,
Mode => 'binmode',
);
if ($Content) {
$MainObject->Require('Text::Diff');
my $Diff = Text::Diff::diff( \$File, $Content, { STYLE => 'OldStyle' } );
$LayoutObject->Block(
Name => "FileDiff",
Data => {
Location => $Location,
Name => $Name,
Version => $Version,
Diff => $Diff,
},
);
}

In order to remove Text::Diff, this would have to be rewritten. A suggestion by @bschmalhofer was to use Algorithm::Diff, which is already included as dependency of Text::Diff:

sub BundleModulesDeclarationGet {
my ($Self) = @_;
return (
{
'Comment' => 'Needed by Text::Diff',
'Module' => 'Algorithm::Diff',
'Required' => 1,
'VersionRequired' => '== 1.1903',
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tidying Tidying of the code
Projects
None yet
Development

No branches or pull requests

2 participants