Bug 36297 - Avoid using var named "t" in templates
Summary: Avoid using var named "t" in templates
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL: https://github.com/abw/Template2/issu...
Keywords:
Depends on: 36158
Blocks:
  Show dependency treegraph
 
Reported: 2024-03-12 09:19 UTC by Fridolin Somers
Modified: 2024-03-19 09:29 UTC (History)
5 users (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 Fridolin Somers 2024-03-12 09:19:19 UTC
Issue in templates discovered in Bug 36158
Avoid using var named "t" in templates (in FOREACH) because of conflict with t() for translations.

 > git grep 'FOREACH t IN'
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_subfields_structure.tt:                                [%- FOREACH t IN [ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10'] -%]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt:                                                            [%- FOREACH t IN [ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'] -%]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt:                        [% FOREACH t IN item_templates.owned %]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt:                        [% FOREACH t IN item_templates.shared %]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt:                    [% FOREACH t IN item_templates.owned %]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt:                            [% FOREACH t IN item_templates.shared %]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs.tt:                                [% FOREACH t IN club_templates %]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs.tt:                                [% FOREACH t IN club_templates %]
origin/master:koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course.tt:                                    [% FOREACH t IN terms %]
Comment 1 Pedro Amorim 2024-03-12 17:56:52 UTC
This may be a non-issue (?) if it is indeed a template toolkit bug and it gets fixed upstream, the problem is that the value of 't' persists even after the end of the loop.
Having said that, we may also just decide that using 't' as a variable name (anywhere) in TT is bad practice, because it may conflict with the translation method 't' we use.
Comment 2 Kyle M Hall 2024-03-14 10:37:27 UTC
(In reply to Pedro Amorim from comment #1)
> This may be a non-issue (?) if it is indeed a template toolkit bug and it
> gets fixed upstream, the problem is that the value of 't' persists even
> after the end of the loop.
> Having said that, we may also just decide that using 't' as a variable name
> (anywhere) in TT is bad practice, because it may conflict with the
> translation method 't' we use.

I don't think we can count on this being fixed upstream any time soon. I think it's reasonable for use to fix the existing instances and perhaps add a unit test to grep for "FOREACH t IN" in all our templates.
Comment 3 Pedro Amorim 2024-03-14 15:04:01 UTC
(In reply to Kyle M Hall from comment #2)
> (In reply to Pedro Amorim from comment #1)
> > This may be a non-issue (?) if it is indeed a template toolkit bug and it
> > gets fixed upstream, the problem is that the value of 't' persists even
> > after the end of the loop.
> > Having said that, we may also just decide that using 't' as a variable name
> > (anywhere) in TT is bad practice, because it may conflict with the
> > translation method 't' we use.
> 
> I don't think we can count on this being fixed upstream any time soon. I
> think it's reasonable for use to fix the existing instances and perhaps add
> a unit test to grep for "FOREACH t IN" in all our templates.

+1
Comment 4 Julian Maurice 2024-03-14 15:21:14 UTC
Remember that there is also 'tn', 'tnx', ... (exhaustive list in i18n.inc).

Also FOREACH is probably not the only way to "hide" these macros (for instance [% t = ... %] or [% WHILE (t = ...) %] probably does the same thing)

Another solution could be to remove i18n.inc, forcing us to use [% I18N.t("...") %] which is unlikely to conflict with other variables.

Or another (but hypothetical as I'm not sure it works): redefine these functions as filters [% "..." | t %] (I believe filters don't conflict with variable names ?)
Comment 5 Kyle M Hall 2024-03-15 11:27:55 UTC
(In reply to Julian Maurice from comment #4)
> Remember that there is also 'tn', 'tnx', ... (exhaustive list in i18n.inc).
> 
> Also FOREACH is probably not the only way to "hide" these macros (for
> instance [% t = ... %] or [% WHILE (t = ...) %] probably does the same thing)
> 
> Another solution could be to remove i18n.inc, forcing us to use [%
> I18N.t("...") %] which is unlikely to conflict with other variables.
> 
> Or another (but hypothetical as I'm not sure it works): redefine these
> functions as filters [% "..." | t %] (I believe filters don't conflict with
> variable names ?)

Further testing leads me to believe that you are correct that filters do not overwrite variable when they have matching names!
Comment 6 Pedro Amorim 2024-03-15 12:52:09 UTC
(In reply to Julian Maurice from comment #4)
> Remember that there is also 'tn', 'tnx', ... (exhaustive list in i18n.inc).
> 
> Also FOREACH is probably not the only way to "hide" these macros (for
> instance [% t = ... %] or [% WHILE (t = ...) %] probably does the same thing)
> 
> Another solution could be to remove i18n.inc, forcing us to use [%
> I18N.t("...") %] which is unlikely to conflict with other variables.
> 
> Or another (but hypothetical as I'm not sure it works): redefine these
> functions as filters [% "..." | t %] (I believe filters don't conflict with
> variable names ?)

These are all good points, although I'm not sure how feasible or cost-efficient would be to turn all occurrences of [% t('translate this') | html %]
into
[% 'translate this' | t | html %]
Running the following:
git grep "\bt(" /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/ | wc -l
Results in 1701 and that's likely not all of it.
I'm assuming we can chain filters in .tt although I'm not 100% sure.
If my above interpretation of what's being suggested is incorrect then please ignore.

git grep "WHILE"
We can see that WHILE is hardly ever used and although it'd be great to ensure it doesn't happen in the future, I'm not sure it's worth the effort at this point to address for this possibility too.

git grep "\[% t="
git grep "\[% t ="
Don't return anything.

Thinking of cost-efficiency here, I think that only considering "FOREACH t IN" in our qa script for now would be a good step in the right direction.
Addressing all possible macro names from i18n may also be an option but I'm not sure how far the rabbit hole goes.
As far as I can tell this issue does not happen anywhere else in Koha at the moment. Even in files like additem.tt, I think because all t("translatable string") calls happen at the start of the file, before any of the "FOREACH t in templates" occurrences that come further down.
Comment 7 Fridolin Somers 2024-03-15 13:45:02 UTC
(In reply to Julian Maurice from comment #4)
> Another solution could be to remove i18n.inc, forcing us to use [%
> I18N.t("...") %] which is unlikely to conflict with other variables.

I would prefer this option.
This will also make is more easy to find translated strings.

But I agree with cost-efficiency ;)
Comment 8 Julian Maurice 2024-03-15 14:45:59 UTC
(In reply to Julian Maurice from comment #4)
> Or another (but hypothetical as I'm not sure it works): redefine these
> functions as filters [% "..." | t %] (I believe filters don't conflict with
> variable names ?)
For the record, it can work as a filter. It requires some code modification that I will submit in another bug report. I think it can be useful whatever we decide on this bug.
Comment 9 Kyle M Hall 2024-03-18 10:42:44 UTC
(In reply to Fridolin Somers from comment #7)
> (In reply to Julian Maurice from comment #4)
> > Another solution could be to remove i18n.inc, forcing us to use [%
> > I18N.t("...") %] which is unlikely to conflict with other variables.
> 
> I would prefer this option.
> This will also make is more easy to find translated strings.
> 
> But I agree with cost-efficiency ;)

If everyone agrees this is the solution, we should be able to make a script to generate the changes. Is anyone opposed to the removal of i18n.inc?
Comment 10 Julian Maurice 2024-03-19 08:31:46 UTC
(In reply to Julian Maurice from comment #8)
> For the record, it can work as a filter. It requires some code modification
> that I will submit in another bug report. 
=> bug 36357