Created attachment 163380 [details] [review] Bug 36357: Add Template::Toolkit filters for internationalization (I18N) This patch makes it possible to write in templates the following: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', count) %] [% 'singular with {var}' | tnx('plural with {var}', cnt, var = 'val') %] These filters are an alternative to the macros defined in i18n.inc. Some advantages: * They do not conflict with variables (see bug 36927) * They are always defined (no need to USE a plugin or PROCESS a file) i18n functions that takes a context argument (tp, tpx, tnp, tnpx) are deliberately not defined as filters. Because context is the first argument, this would require writing: [% 'context' | tp('message') %] Whereas we'd prefer to write: [% 'message' | tp('context') %] There are two ways to fix this: 1. Change the order of arguments in Koha::Template::Plugin::I18N and change the -k options in xgettext-tt2 (the msgid must be first), or 2. Use different keywords, and add them as -k options in xgettext-tt2 Test plan: 1. Find a template file that you can edit and where you can easily see the changes (for instance intranet-main.tt) 2. Add the following code: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', 1) %] [% 'singular with {var}' | tnx('plural with {var}', 2, var = 'val') %] 3. Run misc/translator/translate update fr-FR 4. Open misc/translator/po/fr-FR-messages.po, make sure you find those 4 new strings. Translate them. 5. Run misc/translator/translate install fr-FR 6. Restart Koha 7. Change the language in the interface. Verify that the strings are translated
Created attachment 163381 [details] [review] Bug 36357: Add Template::Toolkit filters for internationalization (I18N) This patch makes it possible to write in templates the following: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', count) %] [% 'singular with {var}' | tnx('plural with {var}', cnt, var = 'val') %] These filters are an alternative to the macros defined in i18n.inc. Some advantages: * They do not conflict with variables (see bug 36297) * They are always defined (no need to USE a plugin or PROCESS a file) i18n functions that takes a context argument (tp, tpx, tnp, tnpx) are deliberately not defined as filters. Because context is the first argument, this would require writing: [% 'context' | tp('message') %] Whereas we'd prefer to write: [% 'message' | tp('context') %] There are two ways to fix this: 1. Change the order of arguments in Koha::Template::Plugin::I18N and change the -k options in xgettext-tt2 (the msgid must be first), or 2. Use different keywords, and add them as -k options in xgettext-tt2 Test plan: 1. Find a template file that you can edit and where you can easily see the changes (for instance intranet-main.tt) 2. Add the following code: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', 1) %] [% 'singular with {var}' | tnx('plural with {var}', 2, var = 'val') %] 3. Run misc/translator/translate update fr-FR 4. Open misc/translator/po/fr-FR-messages.po, make sure you find those 4 new strings. Translate them. 5. Run misc/translator/translate install fr-FR 6. Restart Koha 7. Change the language in the interface. Verify that the strings are translated
Created attachment 163521 [details] [review] Bug 36357: Add Template::Toolkit filters for internationalization (I18N) This patch makes it possible to write in templates the following: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', count) %] [% 'singular with {var}' | tnx('plural with {var}', cnt, var = 'val') %] These filters are an alternative to the macros defined in i18n.inc. Some advantages: * They do not conflict with variables (see bug 36297) * They are always defined (no need to USE a plugin or PROCESS a file) i18n functions that takes a context argument (tp, tpx, tnp, tnpx) are deliberately not defined as filters. Because context is the first argument, this would require writing: [% 'context' | tp('message') %] Whereas we'd prefer to write: [% 'message' | tp('context') %] There are two ways to fix this: 1. Change the order of arguments in Koha::Template::Plugin::I18N and change the -k options in xgettext-tt2 (the msgid must be first), or 2. Use different keywords, and add them as -k options in xgettext-tt2 Test plan: 1. Find a template file that you can edit and where you can easily see the changes (for instance intranet-main.tt) 2. Add the following code: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', 1) %] [% 'singular with {var}' | tnx('plural with {var}', 2, var = 'val') %] 3. Run misc/translator/translate update fr-FR 4. Open misc/translator/po/fr-FR-messages.po, make sure you find those 4 new strings. Translate them. 5. Run misc/translator/translate install fr-FR 6. Restart Koha 7. Change the language in the interface. Verify that the strings are translated Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 163522 [details] [review] Bug 36357: Tidy xgettext-tt2.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 163523 [details] [review] Bug 36357: Tidy I18N.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 163524 [details] [review] Bug 36357: Add POD to Filters/I18N.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Getting Can't locate object method "new" via package "Template::Filters" (perhaps you forgot to load "Template::Filters"?) at /kohadevbox/koha/C4/Templates.pm line 74
(In reply to Kyle M Hall from comment #7) > Getting > Can't locate object method "new" via package "Template::Filters" (perhaps > you forgot to load "Template::Filters"?) at /kohadevbox/koha/C4/Templates.pm > line 74 How do you get that ? I can't reproduce
Created attachment 163597 [details] [review] Bug 36357: Fix Template::Filters error Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Julian Maurice from comment #8) > (In reply to Kyle M Hall from comment #7) > > Getting > > Can't locate object method "new" via package "Template::Filters" (perhaps > > you forgot to load "Template::Filters"?) at /kohadevbox/koha/C4/Templates.pm > > line 74 > > How do you get that ? I can't reproduce Odd. I was able to fix the error by adding "use Template::Filters" to Templates.pm!
Created attachment 168955 [details] [review] Bug 36357: Add Template::Toolkit filters for internationalization (I18N) This patch makes it possible to write in templates the following: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', count) %] [% 'singular with {var}' | tnx('plural with {var}', cnt, var = 'val') %] These filters are an alternative to the macros defined in i18n.inc. Some advantages: * They do not conflict with variables (see bug 36297) * They are always defined (no need to USE a plugin or PROCESS a file) i18n functions that takes a context argument (tp, tpx, tnp, tnpx) are deliberately not defined as filters. Because context is the first argument, this would require writing: [% 'context' | tp('message') %] Whereas we'd prefer to write: [% 'message' | tp('context') %] There are two ways to fix this: 1. Change the order of arguments in Koha::Template::Plugin::I18N and change the -k options in xgettext-tt2 (the msgid must be first), or 2. Use different keywords, and add them as -k options in xgettext-tt2 Test plan: 1. Find a template file that you can edit and where you can easily see the changes (for instance intranet-main.tt) 2. Add the following code: [% 'simple text' | t %] [% 'text with {var}' | tx(var = 'value') %] [% 'singular' | tn('plural', 1) %] [% 'singular with {var}' | tnx('plural with {var}', 2, var = 'val') %] 3. Run misc/translator/translate update fr-FR 4. Open misc/translator/po/fr-FR-messages.po, make sure you find those 4 new strings. Translate them. 5. Run misc/translator/translate install fr-FR 6. Restart Koha 7. Change the language in the interface. Verify that the strings are translated Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 168956 [details] [review] Bug 36357: Tidy xgettext-tt2.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 168957 [details] [review] Bug 36357: Tidy I18N.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 168958 [details] [review] Bug 36357: Add POD to Filters/I18N.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 168959 [details] [review] Bug 36357: Fix Template::Filters error Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
What's the reason to choose `require Koha::Template::Filters::I18N;` vs `use`? See https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL31:_Prefer_.22use.22_than_.22require.22 ---- typos found: «Usesdf different» «Instanticate» --- Besides that, all seems to comply with coding guidelines :) Additional testing was done to double check the plural and single cases. To complement the test plan, I tested in the OPAC instead of staff interface. I don't see what else would be missing from test plan. I don't know enough on the functional side and about eventual unwanted side effects but at least here is a partial QA on the boring steps ^^
Created attachment 168962 [details] [review] Bug 36357: Fix typos
Created attachment 168963 [details] [review] Bug 36357: Use "use" instead of "require"
(In reply to Victor Grousset/tuxayo from comment #16) > What's the reason to choose `require Koha::Template::Filters::I18N;` vs > `use`? It was only to avoid loading the module when not needed. I changed it since it isn't allowed by the new guideline. Thanks for the review
> It was only to avoid loading the module when not needed. I changed it since it isn't allowed by the new guideline. If the loading when not needed is heavy enough, there would be a case to use require. Anyway, thanks for the fixes :)
To be honest I would prefer to have only one solution. If we decide to move to this one, then adjust the previous occurrences. Would you agree with that, Julian?
And we will need to adjust the wiki page: https://wiki.koha-community.org/wiki/Internationalization,_plural_forms,_context,_and_more_RFC
(In reply to Jonathan Druart from comment #21) > To be honest I would prefer to have only one solution. If we decide to move > to this one, then adjust the previous occurrences. Would you agree with > that, Julian? Then we would need to also add the "context variants" (see commit message). Not a problem, just a little more work. I'm not sure I completely agree though. Functions can be useful where filters cannot be easily used. But we can drop the MACROs that pollute global namespace and keep functions under the i18n namespace. Filters would be always available (so they would become naturally the preferred method), and, only when needed, we could USE I18N and its functions. It's not a strong opinion. If more people believe they should be removed, I'm fine with it.