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 %]
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.
(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.
(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
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 ?)
(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!
(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.
(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 ;)
(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.
(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?
(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