Bug 35043

Summary: Handling of \t in PO files is confusing
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00
Circulation function:
Bug Depends on: 34959    
Bug Blocks:    
Attachments: Bug 35043: Use Locale::PO->quote
Bug 35043: Do not have \n or \t appear in PO files
Bug 35043: Use Locale::PO->quote
Bug 35043: Do not have \n or \t appear in PO files
Bug 35043: Use Locale::PO->quote
Bug 35043: Do not have \n or \t appear in PO files

Description Jonathan Druart 2023-10-12 10:33:11 UTC
We have our own "quote_po" sub that is stating that Locale::PO::quote is buggy because it does not deal with quoting new lines correctly. However it seems that it is fixed now.

Ideally we could use Locale::PO::quote everywhere, but it does not escape tab characters:
  $string =~ s/\\(?!t)/\\\\/g;           # \t is a tab

This means the following:
msgid "Tabulation (\\t)"
msgstr "Tabulation (\\t)"
become:
-msgid "Tabulation (\t)"
-msgstr "Tabulation (\t)"

And we are seeing the following on Weblate: https://snipboard.io/BjQmDC.jpg

Note that Locale::PO has not been updated since 2014...
Comment 1 Jonathan Druart 2023-10-12 10:45:03 UTC
Created attachment 156937 [details] [review]
Bug 35043: Use Locale::PO->quote

We have our own "quote_po" sub that is stating that Locale::PO::quote is buggy
because it does not deal with quoting new lines correctly. However it seems that
it is fixed now.

Ideally we could use Locale::PO::quote everywhere, but it does not escape tab
characters:
  $string =~ s/\\(?!t)/\\\\/g;           # \t is a tab

This means the following:
msgid "Tabulation (\\t)"
msgstr "Tabulation (\\t)"
become:
-msgid "Tabulation (\t)"
-msgstr "Tabulation (\t)"

And we are seeing the following on Weblate: https://snipboard.io/BjQmDC.jpg

Note that Locale::PO has not been updated since 2014...

The real problem behind this is that we have 2 methods to quote strings.
At first glance it seems that Locale::PO::quote was not used before, but
with the introduction of the koha-i18n project we will have scripts that
will use Locale::PO->save_file_fromarray, which uses Locale::PO->quote
=> Those scripts will be used on the translation server for post
processing (security reason, marking potential XSS strings as fuzzy).

Test plan:
0. Do not apply the patch
1. gulp po:update --lang LANG # Replace LANG with your favorite language code
2. git commit -a -m"init PO files"
3. Apply this patch
4. Repeate 1.
5. git diff
=> The change is about the "Tabulation" and "New line" strings from tools/csv-profiles.tt
6. Translate them (replace the \t and \n with %s) and remove the fuzzy
   flag
7. install the template: cd misc/translator && perl translate install LANG
8. Enable the language, use it and go to the "Nouveau profil CSV" view
=> Notice that the \t and \n are correctly displayed.
Comment 2 Jonathan Druart 2023-10-12 10:45:06 UTC
Created attachment 156938 [details] [review]
Bug 35043: Do not have \n or \t appear in PO files

This is a trick, so that we won't have to deal with \t in PO files.
\n is not a problem, but it seems consistent to use the same method for
both.
Comment 3 Owen Leonard 2023-10-12 11:05:39 UTC
Created attachment 156950 [details] [review]
Bug 35043: Use Locale::PO->quote

We have our own "quote_po" sub that is stating that Locale::PO::quote
is buggy because it does not deal with quoting new lines correctly.
However it seems that it is fixed now.

Ideally we could use Locale::PO::quote everywhere, but it does not
escape tab characters:
  $string =~ s/\\(?!t)/\\\\/g;           # \t is a tab

This means the following:
msgid "Tabulation (\\t)"
msgstr "Tabulation (\\t)"
become:
-msgid "Tabulation (\t)"
-msgstr "Tabulation (\t)"

And we are seeing the following on Weblate:
https://snipboard.io/BjQmDC.jpg

Note that Locale::PO has not been updated since 2014...

The real problem behind this is that we have 2 methods to quote strings.
At first glance it seems that Locale::PO::quote was not used before, but
with the introduction of the koha-i18n project we will have scripts that
will use Locale::PO->save_file_fromarray, which uses Locale::PO->quote
=> Those scripts will be used on the translation server for post
processing (security reason, marking potential XSS strings as fuzzy).

Test plan:
0. Do not apply the patch
1. gulp po:update --lang LANG # Replace LANG with your favorite language
   code
2. git commit -a -m"init PO files"
3. Apply this patch
4. Repeate 1.
5. git diff
   => The change is about the "Tabulation" and "New line" strings from
      tools/csv-profiles.tt
6. Translate them (replace the \t and \n with %s) and remove the fuzzy
   flag
7. install the template: cd misc/translator && perl translate install
   LANG
8. Enable the language, use it and go to the "Nouveau profil CSV" view
   => Notice that the \t and \n are correctly displayed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Owen Leonard 2023-10-12 11:05:41 UTC
Created attachment 156951 [details] [review]
Bug 35043: Do not have \n or \t appear in PO files

This is a trick, so that we won't have to deal with \t in PO files.
\n is not a problem, but it seems consistent to use the same method for
both.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Marcel de Rooy 2023-10-26 12:41:06 UTC
(In reply to Jonathan Druart from comment #1)
> However it seems
> that
> it is fixed now.

Seems?
My version on D11:
our $VERSION = '0.27';

Do we know at which point this was solved?
Comment 6 Marcel de Rooy 2023-10-26 12:41:45 UTC
Created attachment 157922 [details] [review]
Bug 35043: Use Locale::PO->quote

We have our own "quote_po" sub that is stating that Locale::PO::quote
is buggy because it does not deal with quoting new lines correctly.
However it seems that it is fixed now.

Ideally we could use Locale::PO::quote everywhere, but it does not
escape tab characters:
  $string =~ s/\\(?!t)/\\\\/g;           # \t is a tab

This means the following:
msgid "Tabulation (\\t)"
msgstr "Tabulation (\\t)"
become:
-msgid "Tabulation (\t)"
-msgstr "Tabulation (\t)"

And we are seeing the following on Weblate:
https://snipboard.io/BjQmDC.jpg

Note that Locale::PO has not been updated since 2014...

The real problem behind this is that we have 2 methods to quote strings.
At first glance it seems that Locale::PO::quote was not used before, but
with the introduction of the koha-i18n project we will have scripts that
will use Locale::PO->save_file_fromarray, which uses Locale::PO->quote
=> Those scripts will be used on the translation server for post
processing (security reason, marking potential XSS strings as fuzzy).

Test plan:
0. Do not apply the patch
1. gulp po:update --lang LANG # Replace LANG with your favorite language
   code
2. git commit -a -m"init PO files"
3. Apply this patch
4. Repeate 1.
5. git diff
   => The change is about the "Tabulation" and "New line" strings from
      tools/csv-profiles.tt
6. Translate them (replace the \t and \n with %s) and remove the fuzzy
   flag
7. install the template: cd misc/translator && perl translate install
   LANG
8. Enable the language, use it and go to the "Nouveau profil CSV" view
   => Notice that the \t and \n are correctly displayed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2023-10-26 12:41:47 UTC
Created attachment 157923 [details] [review]
Bug 35043: Do not have \n or \t appear in PO files

This is a trick, so that we won't have to deal with \t in PO files.
\n is not a problem, but it seems consistent to use the same method for
both.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2023-10-27 09:37:20 UTC
(In reply to Marcel de Rooy from comment #5)
> (In reply to Jonathan Druart from comment #1)
> > However it seems
> > that
> > it is fixed now.
> 
> Seems?
> My version on D11:
> our $VERSION = '0.27';
> 
> Do we know at which point this was solved?

Well, if this is 2014, lets assume we are safe.
Comment 9 Tomás Cohen Arazi (tcohen) 2023-10-27 19:52:22 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 10 Fridolin Somers 2023-11-08 07:39:17 UTC
Depends on Bug 34959 not yet in 23.05.x