Bug 35043 - Handling of \t in PO files is confusing
Summary: Handling of \t in PO files is confusing
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 34959
Blocks:
  Show dependency treegraph
 
Reported: 2023-10-12 10:33 UTC by Jonathan Druart
Modified: 2023-11-08 07:39 UTC (History)
2 users (show)

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


Attachments
Bug 35043: Use Locale::PO->quote (4.14 KB, patch)
2023-10-12 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35043: Do not have \n or \t appear in PO files (1.69 KB, patch)
2023-10-12 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35043: Use Locale::PO->quote (4.20 KB, patch)
2023-10-12 11:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35043: Do not have \n or \t appear in PO files (1.74 KB, patch)
2023-10-12 11:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35043: Use Locale::PO->quote (4.29 KB, patch)
2023-10-26 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35043: Do not have \n or \t appear in PO files (1.84 KB, patch)
2023-10-26 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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