Bug 21576: Add a developer script to fix missing TT filters

See bug 13618 and bug 21526.

We need a script to add missing filters, or replace wrong ones.

Test plan:
- Add unescaped variables to .tt files
- prove xt/find-missing-filters.t
will warn about them
- perl misc/devel/add_missing_filters.pl
will add the missing/wrong filters
- prove xt/find-missing-filters.t
will now be happy

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

misc/devel/add_missing_filters.pl

@@ -0,0 +1,93 @@
+#!/usr/bin/perl
+
+use Modern::Perl;
+
+use File::Slurp;
+use Pod::Usage;
+use Getopt::Long;
+
+use t::lib::QA::TemplateFilters;
+
+my ( $help, $verbose, @files );
+GetOptions(
+    'h|help'                 => \$help,
+    'v|verbose'              => \$verbose,
+) || pod2usage(1);
+
+@files = @ARGV;
+
+pod2usage(1) if $help or not @files;
+
+my $i;
+my $total = scalar @files;
+my $num_width = length $total;
+for my $file ( @ARGV ) {
+    if ( $verbose ) {
+        print sprintf "|%-25s| %${num_width}s / %s (%.2f%%)\r",
+            '=' x (24*$i++/$total). '>',
+            $i, $total, 100*$i/+$total;
+        flush STDOUT;
+    }
+
+    my $content = read_file( $file );
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($content);
+    $new_content .= "\n";
+    if ( $content ne $new_content ) {
+        say "$file -- Modified";
+        write_file($file, $new_content);
+    }
+}
+
+
+=head1 NAME
+
+add_missing_filters.pl - Will add the missing filters to the template files given in parameters.
+
+=head1 SYNOPSIS
+
+perl misc/devel/add_missing_filters.pl **/*.tt
+
+/!\ It is highly recommended to execute this script on a clean git install, with all your files and changes committed.
+
+ Options:
+   -?|--help        brief help message
+   -v|--verbose     verbose mode
+
+=head1 OPTIONS
+
+=over 8
+
+=item B<--help|-?>
+
+Print a brief help message and exits
+
+=item B<-v|--verbose>
+
+Verbose mode.
+
+=back
+
+=head1 AUTHOR
+
+Jonathan Druart <jonathan.druart@bugs.koha-community.org>
+
+=head1 COPYRIGHT
+
+Copyright 2018 Koha Development Team
+
+=head1 LICENSE
+
+This file is part of Koha.
+
+Koha is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3 of the License, or
+(at your option) any later version.
+
+Koha is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with Koha; if not, see <http://www.gnu.org/licenses>.

t/lib/QA/TemplateFilters.pm

@@ -27,24 +27,43 @@ our @tt_directives = (
     qr{^\s*LAST},
 );
 
-sub missing_filters {
+sub fix_filters {
+    return _process_tt_content( @_ )->{new_content};
+}
+
+sub search_missing_filters {
+    return _process_tt_content( @_ )->{errors};
+
+}
+
+sub _process_tt_content {
     my ($content) = @_;
     my ( $use_raw, $has_use_raw );
     my @errors;
+    my @new_lines;
     my $line_number;
     for my $line ( split "\n", $content ) {
+        my $new_line = $line;
         $line_number++;
         if ( $line =~ m{\[%[^%]+%\]} ) {
 
             # handle exceptions first
-            $use_raw = 1
-              if $line =~ m{|\s*\$raw};    # Is the file use the raw filter?
+            if ( $line =~ m{\|\s*\$raw} ) {    # Is the file use the raw filter?
+                $use_raw = 1;
+            }
 
             # Do we have Asset without the raw filter?
-            if ( $line =~ m{^\s*\[% Asset} ) {
-                push @errors, { error => 'asset_must_be_raw', line => $line, line_number => $line_number }
-                  and next
-                  unless $line =~ m{\|\s*\$raw};
+            if ( $line =~ m{^\s*\[% Asset} && $line !~ m{\|\s*\$raw} ) {
+                push @errors,
+                  {
+                    error       => 'asset_must_be_raw',
+                    line        => $line,
+                    line_number => $line_number
+                  };
+                $new_line =~ s/\)\s*%]/) | \$raw %]/;
+                $use_raw = 1;
+                push @new_lines, $new_line;
+                next;
             }
 
             $has_use_raw++
@@ -60,29 +79,132 @@ sub missing_filters {
                     %\]}gmxs
               )
             {
-                my $tt_block = $+{tt_block};
+                my $tt_block   = $+{tt_block};
+                my $pre_chomp  = $+{pre_chomp};
+                my $post_chomp = $+{post_chomp};
 
-                # It's a TT directive, no filters needed
-                next if grep { $tt_block =~ $_ } @tt_directives;
+                next if
+                    # It's a TT directive, no filters needed
+                    grep { $tt_block =~ $_ } @tt_directives
 
-                next
-                  if   $tt_block =~ m{\s?\|\s?\$KohaDates\s?$}
+                    # It is a comment
+                    or $tt_block =~ m{^\#}
+
+                    # Already escaped with a special filter
+                    # We could escape it but should be safe
+                    or $tt_block =~ m{\s?\|\s?\$KohaDates\s?$}
                     or $tt_block =~ m{\s?\|\s?\$Price\s?$}
-                  ;    # We could escape it but should be safe
-                next if $tt_block =~ m{^\#};    # Is a comment, skip it
 
-                push @errors, { error => 'missing_filter', line => $line, line_number => $line_number }
-                  if $tt_block !~ m{\|\s?\$raw}   # already escaped correctly with raw
-                  && $tt_block !~ m{=}            # assignment, maybe we should require to use SET (?)
-                  && $tt_block !~ m{\|\s?ur(l|i)} # already has url or uri filter
-                  && $tt_block !~ m{\|\s?html}    # already has html filter
-                  && $tt_block !~ m{^(?<before>\S+)\s+UNLESS\s+(?<after>\S+)} # Specific for [% foo UNLESS bar %]
+                    # Already escaped correctly with raw
+                    or $tt_block =~ m{\|\s?\$raw}
+
+                    # Assignment, maybe we should require to use SET (?)
+                    or $tt_block =~ m{=}
+
+                    # Already has url or uri filter
+                    or $tt_block =~ m{\|\s?ur(l|i)}
+
+                    # Specific for [% foo UNLESS bar %]
+                    or $tt_block =~ m{^(?<before>\S+)\s+UNLESS\s+(?<after>\S+)}
                 ;
+
+                $pre_chomp =
+                    $pre_chomp
+                  ? $pre_chomp =~ m|-|
+                      ? q|- |
+                      : $pre_chomp =~ m|~|
+                        ? q|~ |
+                        : q| |
+                  : q| |;
+                $post_chomp =
+                    $post_chomp
+                  ? $post_chomp =~ m|-|
+                      ? q| -|
+                      : $post_chomp =~ m|~|
+                        ? q| ~|
+                        : q| |
+                  : q| |;
+
+                if (
+
+                    # Use the uri filter
+                    # If html filtered or not filtered
+                    $new_line =~ qr{
+                        <a\shref="(tel:|mailto:)?
+                        \[%
+                            \s*$pre_chomp\s*
+                            \Q$tt_block\E
+                            \s*$post_chomp\s*
+                            (\|\s*html)?
+                            \s*
+                        %\]
+                    }xms
+
+                    # And not already uri or url filtered
+                    and not $new_line =~ qr{
+                        <a\shref="(tel:|mailto:)?
+                        \[%
+                            \s*$pre_chomp\s*
+                            \Q$tt_block\E
+                            \s|\s(uri|url)
+                            \s*$post_chomp\s*
+                        %\]
+                    }xms
+                  )
+                {
+                    $tt_block =~ s/^\s*|\s*$//g;    # trim
+                    $tt_block =~ s/\s*\|\s*html\s*//;
+                    $new_line =~ s{
+                            \[%
+                            \s*$pre_chomp\s*
+                            \Q$tt_block\E(\s*\|\s*html)?
+                            \s*$post_chomp\s*
+                            %\]
+                        }{[%$pre_chomp$tt_block | uri$post_chomp%]}xms;
+                    push @errors,
+                      {
+                        error       => 'wrong_html_filter',
+                        line        => $line,
+                        line_number => $line_number
+                      };
+                    next;
+                }
+                elsif (
+                    $tt_block !~ m{\|\s?html} # already has html filter
+                  )
+                {
+                    $tt_block =~ s/^\s*|\s*$//g; # trim
+                    $new_line =~ s{
+                        \[%
+                        \s*$pre_chomp\s*
+                        \Q$tt_block\E
+                        \s*$post_chomp\s*
+                        %\]
+                    }{[%$pre_chomp$tt_block | html$post_chomp%]}xms;
+
+                    push @errors,
+                      {
+                        error       => 'missing_filter',
+                        line        => $line,
+                        line_number => $line_number
+                      };
+                    next;
+                }
             }
+            push @new_lines, $new_line;
         }
+        else {
+            push @new_lines, $new_line;
+        }
+
     }
 
-    return @errors;
+    # Adding [% USE raw %] on top if the filter is used
+    @new_lines = ( '[% USE raw %]', @new_lines )
+      if $use_raw and not $has_use_raw;
+
+    my $new_content = join "\n", @new_lines;
+    return { errors => \@errors, new_content => $new_content };
 }
 
 1;
@@ -94,7 +216,8 @@ t::lib::QA::TemplateFilters - Module used by tests and QA script to catch missin
 =head1 SYNOPSIS
 
     my $content = read_file($filename);
-    my @e = t::lib::QA::TemplateFilters::missing_filters($content);
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($content);
+    my $errors      = t::lib::QA::TemplateFilters::search_missing_filters($content);
 
 =head1 DESCRIPTION
 
@@ -103,15 +226,26 @@ and to not duplicate the code.
 
 =head1 METHODS
 
-=head2 missing_filters
+=head2 fix_filters
 
-    Take a template content file in parameter and return an array of errors.
-    An error is a hashref with 2 keys, error and line.
+    Take a template content file in parameter and return the same content with
+    the correct (guessed) filters.
+    It will also add the [% USE raw %] statement if it is needed.
+
+=head2 search_missing_filters
+
+    Take a template content file in parameter and return an arrayref of errors.
+
+    An error is a hashref with 3 keys, error and line, line_number.
     * error can be:
     asset_must_be_raw - When Asset is called without using raw
     missing_filter    - When a TT variable is displayed without filter
+    wrong_html_filter - When a TT variable is using the html filter when uri (or url)
+                        should be used instead.
 
     * line is the line where the error has been found.
+    * line_number is the line number where the error has been found.
+
 
 =head1 AUTHORS
 

t/template_filters.t

@@ -16,28 +16,107 @@
 # along with Koha; if not, see <http://www.gnu.org/licenses>.
 
 use Modern::Perl;
-use Test::More tests => 1;
+use Test::More tests => 5;
 use t::lib::QA::TemplateFilters;
 
-my $input = <<INPUT;
-[% USE Asset %]
-[% INCLUDE 'doc-head-open.inc' %]
+subtest 'Asset must use raw' => sub {
+    plan tests => 2;
+    my $input = <<INPUT;
+[% Asset.css("css/one.css") %]
+[% Asset.css("js/two.js") %]
+INPUT
+    my $expected = <<EXPECTED;
+[% USE raw %]
+[% Asset.css("css/one.css") | \$raw %]
+[% Asset.css("js/two.js") | \$raw %]
+EXPECTED
+
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($input);
+    is( $new_content . "\n", $expected, );
+    my $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input);
+    is_deeply(
+        $missing_filters,
+        [
+            {
+                error       => "asset_must_be_raw",
+                line        => '[% Asset.css("css/one.css") %]',
+                line_number => 1,
+            },
+            {
+                error       => "asset_must_be_raw",
+                line        => '[% Asset.css("js/two.js") %]',
+                line_number => 2,
+            }
+
+        ],
+    );
+};
+
+subtest 'Variables must be html escaped' => sub {
+    plan tests => 2;
+
+    my $input = <<INPUT;
 <title>Koha &rsaquo; Patrons &rsaquo;
     [% UNLESS blocking_error %]
-        Patron details for [% INCLUDE 'patron-title.inc' no_html = 1 %]
+        [% just_a_var %]
         [% just_a_var %] A N D [% another_one_on_same_line %]
-        [% just_a_var_filtered|html %]
-        [% just_a_var_filtered |html %]
-        [% just_a_var_filtered| html %]
-        [% just_a_var_filtered | html %]
     [% END %]
-    [% IF ( patron.othernames | html ) %]&ldquo;[% patron.othernames %]&rdquo;[% END %]
-    [% Asset.css("css/datatables.css").raw %]
-    [% Asset.css("css/datatables.css") | \$raw %]
+    [% IF ( patron.othernames ) %]&ldquo;[% patron.othernames %]&rdquo;[% END %]
 </title>
-<a href="tel:[% patron.phone %]">[% patron.phone %]</a>
-<a title="[% patron.emailpro %]" href="mailto:[% patron.emailpro | uri %]">[% patron.emailpro %]</a>
 [% patron_message.get_column('manager_surname') %]
+INPUT
+
+    my $expected = <<EXPECTED;
+<title>Koha &rsaquo; Patrons &rsaquo;
+    [% UNLESS blocking_error %]
+        [% just_a_var | html %]
+        [% just_a_var | html %] A N D [% another_one_on_same_line | html %]
+    [% END %]
+    [% IF ( patron.othernames ) %]&ldquo;[% patron.othernames | html %]&rdquo;[% END %]
+</title>
+[% patron_message.get_column('manager_surname') | html %]
+EXPECTED
+
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($input);
+    is( $new_content . "\n", $expected, );
+    my $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input);
+    is_deeply(
+        $missing_filters,
+        [{
+                error => "missing_filter",
+                line => "        [% just_a_var %]",
+                line_number => 3,
+            },
+            {
+                error => "missing_filter",
+                line => "        [% just_a_var %] A N D [% another_one_on_same_line %]",
+                line_number => 4,
+            },
+            {
+                error => "missing_filter",
+                line => "        [% just_a_var %] A N D [% another_one_on_same_line %]",
+                line_number => 4,
+            },
+            {
+                error => "missing_filter",
+                line => "    [% IF ( patron.othernames ) %]&ldquo;[% patron.othernames %]&rdquo;[% END %]",
+                line_number => 6,
+            },
+            {
+                error => "missing_filter",
+                line  => "[% patron_message.get_column('manager_surname') %]",
+                line_number => 8
+            }
+        ],
+
+    );
+};
+
+subtest 'TT directives, assignments and already filtered variables must not be escaped' => sub {
+    plan tests => 2;
+    my $input = <<INPUT;
+#[% USE Asset %]
+[% INCLUDE 'doc-head-open.inc' %]
 [%# do_nothing %]
 [% # do_nothing %]
 [% SWITCH var %]
@@ -48,90 +127,124 @@ my $input = <<INPUT;
 [%- CASE 'foo' -%]foo
 [%- CASE -%]
 [%- END -%]
-[%- var -%]
-[% - var - %]
-[%~ var ~%]
-[% ~ var ~ %]
-[% var | \$raw %]
 [% foo UNLESS bar %]
 [% SET var = val %]
 [% var = val %]
 [% var | \$Price %]
+[% just_a_var_filtered|html %]
+[% just_a_var_filtered |html %]
+[% just_a_var_filtered| html %]
+[% just_a_var_filtered | html %]
 [%END%]
 INPUT
+    my $expected = <<EXPECTED;
+#[% USE Asset %]
+[% INCLUDE 'doc-head-open.inc' %]
+[%# do_nothing %]
+[% # do_nothing %]
+[% SWITCH var %]
+[% CASE 'foo' %]foo
+[% CASE %]
+[% END %]
+[%- SWITCH var -%]
+[%- CASE 'foo' -%]foo
+[%- CASE -%]
+[%- END -%]
+[% foo UNLESS bar %]
+[% SET var = val %]
+[% var = val %]
+[% var | \$Price %]
+[% just_a_var_filtered|html %]
+[% just_a_var_filtered |html %]
+[% just_a_var_filtered| html %]
+[% just_a_var_filtered | html %]
+[%END%]
+EXPECTED
 
-my @expected_errors = (
-    {
-        error => q{missing_filter},
-        line =>
-q{        [% just_a_var %] A N D [% another_one_on_same_line %]},
-        line_number => 6,
-    },
-    {
-        error => q{missing_filter},
-        line =>
-q{        [% just_a_var %] A N D [% another_one_on_same_line %]},
-        line_number => 6,
-    },
-    {
-        error => q{missing_filter},
-        line =>
-q{    [% IF ( patron.othernames | html ) %]&ldquo;[% patron.othernames %]&rdquo;[% END %]},
-        line_number => 12,
-    },
-    {
-        error       => q{asset_must_be_raw},
-        line        => q{    [% Asset.css("css/datatables.css").raw %]},
-        line_number => 13,
-    },
-    {
-        error => q{missing_filter},
-        line  => q{<a href="tel:[% patron.phone %]">[% patron.phone %]</a>},
-        line_number => 16,
-    },
-    {
-        error => q{missing_filter},
-        line  => q{<a href="tel:[% patron.phone %]">[% patron.phone %]</a>},
-        line_number => 16,
-    },
-    {
-        error => q{missing_filter},
-        line =>
-q{<a title="[% patron.emailpro %]" href="mailto:[% patron.emailpro | uri %]">[% patron.emailpro %]</a>},
-        line_number => 17,
-    },
-    {
-        error => q{missing_filter},
-        line =>
-q{<a title="[% patron.emailpro %]" href="mailto:[% patron.emailpro | uri %]">[% patron.emailpro %]</a>},
-        line_number => 17,
-    },
-    {
-        error       => q{missing_filter},
-        line        => q{[% patron_message.get_column('manager_surname') %]},
-        line_number => 18,
-    },
-    {
-        error       => q{missing_filter},
-        line        => q{[%- var -%]},
-        line_number => 29,
-    },
-    {
-        error       => q{missing_filter},
-        line        => q{[% - var - %]},
-        line_number => 30,
-    },
-    {
-        error       => q{missing_filter},
-        line        => q{[%~ var ~%]},
-        line_number => 31,
-    },
-    {
-        error       => q{missing_filter},
-        line        => q{[% ~ var ~ %]},
-        line_number => 32,
-    }
-);
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($input);
+    is( $new_content . "\n", $expected, );
+    my $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input);
+    is_deeply(
+        $missing_filters,[],);
+};
 
-my @get = t::lib::QA::TemplateFilters::missing_filters($input);
-is_deeply( \@get, \@expected_errors);
+subtest 'Preserve pre/post chomps' => sub {
+    plan tests => 1;
+    my $input = <<INPUT;
+[% USE raw %]
+[%- var -%]
+[% - var - %]
+[%~ var ~%]
+[% ~ var ~ %]
+[%- var | html -%]
+[%~ var | html ~%]
+[%- var | uri -%]
+[%~ var | uri ~%]
+INPUT
+    my $expected = <<EXPECTED;
+[% USE raw %]
+[%- var | html -%]
+[%- var | html -%]
+[%~ var | html ~%]
+[%~ var | html ~%]
+[%- var | html -%]
+[%~ var | html ~%]
+[%- var | uri -%]
+[%~ var | uri ~%]
+EXPECTED
+
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($input);
+    is( $new_content . "\n", $expected, );
+};
+
+subtest 'Use uri filter if needed' => sub {
+    plan tests => 3;
+    my $input = <<INPUT;
+<a href="tel:[% patron.phone %]">[% patron.phone %]</a>
+<a href="mailto:[% patron.emailpro %]" title="[% patron.emailpro %]">[% patron.emailpro %]</a>
+<a href="mailto:[% patron.emailpro | html %]" title="[% patron.emailpro %]">[% patron.emailpro %]</a>
+<a href="mailto:[% patron.emailpro | uri %]" title="[% patron.emailpro %]">[% patron.emailpro %]</a>
+<a href="[% myuri %]" title="[% myuri %]">[% myuri %]</a>
+<a href="[% myuri | uri %]" title="[% myuri %]">[% myuri %]</a>
+<a href="[% myurl | html %]" title="[% myurl %]">[% myurl %]</a>
+<a href="[% myurl | url %]" title="[% myurl %]">[% myurl %]</a>
+INPUT
+
+    # Note: [% myurl %] will be uri escaped, we cannot know url should be used
+    my $expected = <<EXPECTED;
+<a href="tel:[% patron.phone | uri %]">[% patron.phone | html %]</a>
+<a href="mailto:[% patron.emailpro | uri %]" title="[% patron.emailpro | html %]">[% patron.emailpro | html %]</a>
+<a href="mailto:[% patron.emailpro | uri %]" title="[% patron.emailpro | html %]">[% patron.emailpro | html %]</a>
+<a href="mailto:[% patron.emailpro | uri %]" title="[% patron.emailpro | html %]">[% patron.emailpro | html %]</a>
+<a href="[% myuri | uri %]" title="[% myuri | html %]">[% myuri | html %]</a>
+<a href="[% myuri | uri %]" title="[% myuri | html %]">[% myuri | html %]</a>
+<a href="[% myurl | uri %]" title="[% myurl | html %]">[% myurl | html %]</a>
+<a href="[% myurl | url %]" title="[% myurl | html %]">[% myurl | html %]</a>
+EXPECTED
+
+    my $new_content = t::lib::QA::TemplateFilters::fix_filters($input);
+    is( $new_content . "\n", $expected, );
+
+    $input = <<INPUT;
+<a href="[% wrong_filter | html %]">[% var | html %]</a>
+INPUT
+    my $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input);
+    is_deeply(
+        $missing_filters,
+        [
+            {
+                error => "wrong_html_filter",
+                line =>
+                  '<a href="[% wrong_filter | html %]">[% var | html %]</a>',
+                line_number => 1
+            }
+
+        ],
+    );
+
+    $input = <<INPUT;
+<a href="[% good_raw_filter | \$raw %]">[% var | html %]</a>
+INPUT
+    $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input);
+    is_deeply( $missing_filters, [], );
+};

xt/find-missing-filters.t

@@ -52,8 +52,8 @@ find({ wanted => \&wanted, no_chdir => 1 }, @themes );
 my @errors;
 for my $file ( @files ) {
     my $content = read_file($file);
-    my @e = t::lib::QA::TemplateFilters::missing_filters($content);
-    push @errors, { file => $file, errors => \@e } if @e;
+    my $e = t::lib::QA::TemplateFilters::search_missing_filters($content);
+    push @errors, { file => $file, errors => $e } if @$e;
 }
 
 is( @errors, 0, "Template variables should be correctly escaped" )