I was testing a patch (on Bug 16525) which contained this markup: <a href="[% IF ( budget_period_id) %]/cgi-bin/koha/admin/aqbudgets.pl?budget_period_id=[% budget_period_id %][% ELSE %]/cgi-bin/koha/admin/aqbudgetperiods.pl[% END %]" class="cancel">Cancel</a> I expected it to trigger a warning by the QA tools. My system is a gitified package install, so it seems possible that this is a problem with my configuration. Running "prove xt" doesn't catch it either.
xt/tt_valid.t and the QA tools do not caught this invalid syntax because the TT tags ([% %]) are inside the value of the attribute (surrounded by quotes). It could be possible to add a test for the 'selected' attributes, but it would not be exhaustive.
Owen, note that the changes made by bug 16525 do not introduce translatable issues. With a quick hack to xt/tt_valid.t, I get: # intranet-tmpl/prog/en/includes/doc-head-close-receipt.inc: 2 # intranet-tmpl/prog/en/includes/doc-head-close.inc: 19 # intranet-tmpl/prog/en/includes/facets.inc: 6, 6, 45 # intranet-tmpl/prog/en/includes/help-top.inc: 4 # intranet-tmpl/prog/en/includes/page-numbers.inc: 3, 4, 6 # intranet-tmpl/prog/en/modules/acqui/basket.tt: 554, 555, 556, 557, 558, 559, 560, 562 # intranet-tmpl/prog/en/modules/acqui/parcels.tt: 132, 138, 142 # intranet-tmpl/prog/en/modules/acqui/transferorder.tt: 125 # intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt: 383 # intranet-tmpl/prog/en/modules/admin/categories.tt: 92 # intranet-tmpl/prog/en/modules/admin/preferences.tt: 121 # intranet-tmpl/prog/en/modules/authorities/authorities.tt: 321 # intranet-tmpl/prog/en/modules/basket/basket.tt: 6 # intranet-tmpl/prog/en/modules/catalogue/subject.tt: 31 # intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt: 437, 629 # intranet-tmpl/prog/en/modules/circ/printslip.tt: 14 # intranet-tmpl/prog/en/modules/circ/waitingreserves.tt: 139 # intranet-tmpl/prog/en/modules/members/moremember-print.tt: 8 # intranet-tmpl/prog/en/modules/members/moremember-receipt.tt: 6 # intranet-tmpl/prog/en/modules/members/printfeercpt.tt: 7 # intranet-tmpl/prog/en/modules/members/printinvoice.tt: 5 # intranet-tmpl/prog/en/modules/reports/stats_screen.tt: 23, 25 # intranet-tmpl/prog/en/modules/reserve/request.tt: 847 # intranet-tmpl/prog/en/modules/suggestion/suggestion.tt: 458, 459 # intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt: 415, 423, 432 # opac-tmpl/bootstrap/en/includes/doc-head-close.inc: 4 # opac-tmpl/bootstrap/en/includes/opac-facets.inc: 9, 11, 54 # opac-tmpl/bootstrap/en/includes/page-numbers.inc: 5, 11, 15 # opac-tmpl/bootstrap/en/includes/shelfbrowser.inc: 39, 41 # opac-tmpl/bootstrap/en/modules/opac-advsearch.tt: 138 # opac-tmpl/bootstrap/en/modules/opac-detail.tt: 427, 429, 437, 439, 448, 450, 459, 461, 965, 967, 1072, 1081 # opac-tmpl/bootstrap/en/modules/opac-readingrecord.tt: 48, 52 # opac-tmpl/bootstrap/en/modules/opac-review.tt: 41 # opac-tmpl/bootstrap/en/modules/opac-showreviews.tt: 8, 82 # opac-tmpl/bootstrap/en/modules/sco/help.tt: 6 # opac-tmpl/bootstrap/en/modules/sco/printslip.tt: 4 # opac-tmpl/bootstrap/en/modules/sco/sco-main.tt: 7 I had a quick look at them and have not seen any problems with them.
The "quick hack" is diff --git a/xt/tt_valid.t b/xt/tt_valid.t index 7285019..c335bb9 100755 --- a/xt/tt_valid.t +++ b/xt/tt_valid.t @@ -46,9 +46,12 @@ my $checkers = [ description => 'TT syntax: not using TT directive within HTML tag', check => sub { my ($self, $name, $token) = @_; + push @{$self->{errors}->{$name}}, $token->{_lc} if exists $token->{_string} and $token->{_string} =~ m|\[%\s*IF|; my $attr = $token->{_attr}; next unless $attr;
So our coding guideline, "HTML1: Template Toolkit markup inside HTML" is too broad?
(In reply to Owen Leonard from comment #4) > So our coding guideline, "HTML1: Template Toolkit markup inside HTML" is too > broad? To avoid translatability issues, yes. But if you look at the different places where it is used, it does not ease the readability. So I think it's good to keep it like that (unless specific cases where it could be useful) even if we do not have a check to catch them. Or we could fix all the occurrences and add a new check :)
Not sure what's going on here, but is this one still valid?