Bug 16536 - Warn about Template Toolkit directives inside HTML tags
Summary: Warn about Template Toolkit directives inside HTML tags
Status: NEW
Alias: None
Product: Project Infrastructure
Classification: Unclassified
Component: QA tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-17 13:25 UTC by Owen Leonard
Modified: 2022-12-06 15:43 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2016-05-17 13:25:50 UTC
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.
Comment 1 Jonathan Druart 2016-05-17 14:11:33 UTC
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.
Comment 2 Jonathan Druart 2016-05-17 14:27:35 UTC
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.
Comment 3 Jonathan Druart 2016-05-17 14:28:13 UTC
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;
Comment 4 Owen Leonard 2016-05-17 14:36:14 UTC
So our coding guideline, "HTML1: Template Toolkit markup inside HTML" is too broad?
Comment 5 Jonathan Druart 2016-05-17 15:18:49 UTC
(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 :)
Comment 6 David Cook 2022-12-06 06:02:22 UTC
Not sure what's going on here, but is this one still valid?