Bug 9114 - exported MARC frameworks encoding should be UTF-8
Summary: exported MARC frameworks encoding should be UTF-8
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-20 14:19 UTC by Gaetan Boisson
Modified: 2015-06-04 23:30 UTC (History)
6 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:


Attachments
Bug 9114: import/export routines should use UTF-8 (3.12 KB, patch)
2013-11-05 17:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9114: import/export routines should use UTF-8 (3.31 KB, patch)
2014-01-16 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9114: import/export routines should use UTF-8 (3.32 KB, patch)
2014-01-17 16:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9114: Make frameworks import/export routines correctly use UTF-8 (2.55 KB, patch)
2014-01-22 18:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9114: Make frameworks import/export routines correctly use UTF-8 (2.96 KB, patch)
2014-03-13 14:24 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 9114: Make frameworks import/export routines correctly use UTF-8 (2.95 KB, patch)
2014-03-13 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 9114: Make frameworks import/export routines correctly use UTF-8 (3.06 KB, patch)
2014-03-14 13:51 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Gaetan Boisson 2012-11-20 14:19:43 UTC
I have noticed on several instances that when exporting a MARC framework it is encoded in ISO-8859-1, which should be UTF-8, the expected encoding for importing frameworks.

Also, when chossing SQL as the export format, single quotes are escaped with backslashes, but these backslashes are not removed when importing.
Comment 1 Tomás Cohen Arazi 2013-11-05 17:31:48 UTC Comment hidden (obsolete)
Comment 2 Mathieu Saby 2013-12-23 17:58:41 UTC
Hello
WITHOUT your patch, when I run

$ file ~/Téléchargements/export_.csv 
I get:
/home/msaby/Téléchargements/export_.csv: UTF-8 Unicode text

I also put some chinese characters in the name of a field, and when I import the saved framework, I can see those characters in the new framework in Koha.

So, can you confirm the issue, and your test plan?

Mathieu
Comment 3 Marcel de Rooy 2014-01-16 09:06:48 UTC
Tomas: Thank you for your patch. I have a comment however :) You use binmode on STDOUT in the module, but you print to a file handle. So that should not be enough. I would suggest to replace the binmode by adding the encoding to the output file handles. Note that I also had a wide character warning in my log on admin/import_export_framework. You probably need to touch code there as well?
I tested with a Chinese character; that went well (export and re-import). Since you actually leave it to Perl to decide the encoding, Perl made the right choice here. But with some Latin characters only, export and re-import made them disappear..

Failed QA
Comment 4 Marcel de Rooy 2014-01-16 09:09:29 UTC
(In reply to M. de Rooy from comment #3)
> Tomas: Thank you for your patch. I have a comment however :) You use binmode
> on STDOUT in the module, but you print to a file handle. So that should not
> be enough. I would suggest to replace the binmode by adding the encoding to
> the output file handles. Note that I also had a wide character warning in my
> log on admin/import_export_framework. You probably need to touch code there
> as well?
> I tested with a Chinese character; that went well (export and re-import).
> Since you actually leave it to Perl to decide the encoding, Perl made the
> right choice here. But with some Latin characters only, export and re-import
> made them disappear..
> 
> Failed QA

Some confusion: The binmode is not in the module, but printing to the file handle is. So please correct.
Comment 5 Marcel de Rooy 2014-01-16 09:22:27 UTC
Even more confusion: Please forget the former two comments. I am retesting. Something went wrong. The warning on the wide char is not from your patch after all.
Back to Needs Signoff
Comment 6 Marcel de Rooy 2014-01-16 09:42:44 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2014-01-17 16:01:56 UTC Comment hidden (obsolete)
Comment 8 Galen Charlton 2014-01-17 16:43:25 UTC
This patch breaks the ODS format.  ODS is a fancy ZIP file, and therefore needs to be written in binary mode.  I also observed that strings got double-encoded upon export to the Excel XML format.

Marking failed QA for the regressions.

For folks testing future version of this patch, please test importing and exporting all four formats. I also want to highlight a part of Marcel's comment 3: to reproduce the bug, you actually don't want Chinese characters or the like in the framework you're exporting -- you want Western European diacritical characters that are to be found in the ISO-8859-1 character set.  As Marcel observed, if you've got non-Latin1 characters in your string, Perl is much more likely to emit UTF8.  Of course, you should test with non-Latin1 characters too, but they're less likely to trigger the bug.
Comment 9 Marcel de Rooy 2014-01-20 08:57:51 UTC
(In reply to Galen Charlton from comment #8)
> Marking failed QA for the regressions.
thx for scrutiny++
Comment 10 Tomás Cohen Arazi 2014-01-22 15:53:44 UTC
(In reply to Galen Charlton from comment #8)
> This patch breaks the ODS format.  ODS is a fancy ZIP file, and therefore
> needs to be written in binary mode.  I also observed that strings got
> double-encoded upon export to the Excel XML format.

The patch broke the ODS format, I've fixed easily. The Excel XML format is broken on master actually. I'll try to fix it too.

Thanks for the insight Galen.
Comment 11 Tomás Cohen Arazi 2014-01-22 18:45:26 UTC Comment hidden (obsolete)
Comment 12 Magnus Enger 2014-03-13 14:24:10 UTC Comment hidden (obsolete)
Comment 13 Galen Charlton 2014-03-13 16:42:24 UTC
Comment on attachment 26285 [details] [review]
Bug 9114: Make frameworks import/export routines correctly use UTF-8

Review of attachment 26285 [details] [review]:
-----------------------------------------------------------------

::: admin/import_export_framework.pl
@@ +56,5 @@
>      ExportFramework($frameworkcode, \$strXml, $format);
> +
> +    # Correctly set the encoding to output plain text in UTF-8
> +    if ( $format eq 'csv' ||
> +         $format eq 'sql' ) {

Note that 'sql' was removed as an export option in security bug 11666.
Comment 14 Tomás Cohen Arazi 2014-03-13 17:16:22 UTC
(In reply to Galen Charlton from comment #13)
> Comment on attachment 26285 [details] [review] [review]
> Bug 9114: Make frameworks import/export routines correctly use UTF-8
> 
> Review of attachment 26285 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: admin/import_export_framework.pl
> @@ +56,5 @@
> >      ExportFramework($frameworkcode, \$strXml, $format);
> > +
> > +    # Correctly set the encoding to output plain text in UTF-8
> > +    if ( $format eq 'csv' ||
> > +         $format eq 'sql' ) {
> 
> Note that 'sql' was removed as an export option in security bug 11666.

I'll attach a new and correct patch.
Comment 15 Tomás Cohen Arazi 2014-03-13 18:25:20 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2014-03-14 13:51:24 UTC
Created attachment 26350 [details] [review]
[PASSED QA] Bug 9114: Make frameworks import/export routines correctly use UTF-8

Currently both the import_export_framework.pl script outputs data with
Perl's default encoding, ISO-8859. This patch properly sets
the binmode to UTF-8 when exporting SQL and CSV files using the PerlIO layer
(":encoding(UTF-8)") for STDOUT.

To test:

Export step test
- Use some ASCII character(s) with DIACRITICS in some field description in a chosen framework.
- Export the framework at Administration > MARC frameworks
- Run this to check the file is ISO-8859 encoded:
 $ file export_XXX.csv
 export_XXX.csv: ISO-8859 text, with very long lines
 (Note: try SQL and other output formats too. But not ODS)
- Apply the patch
- Export the framework again (change the name), and test encoding:
 $ file export_XXX_2.csv
 export_XXX_2.csv: UTF-8 Unicode text

Import step test
I assume you have two files, export_XXX.csv (ISO-8859 encoded) and export_XXX_2.csv (XXX will depend on
your framework's code)
- Reset your testing branch to master
- Import export_XXX.csv
- The string with non-ASCII chars is truncated at the first non-ASCII char's position
  (Note: this is the current behaviour).
- Import export_XXX_2.csv
- The non-ASCII chars are broken, the logs show errors on non-UNICODE chars.
  (Note: even thou UTF-8 is the expected encoding it is treated as ISO-8859).
- Apply the patch
- Import the good (UTF-8 as expected) file and check everything worked as expected.

No double encoding should occur with either combination of formats.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Magnus Enger <digitalutvikling@gmail.com>
I put some Norwegian and accented letters in a fremawork to test.
Before the patch, the exported CSV came out as ISO-8859, after the
patch it came out as UTF-8. ODS and XML (viewed in LibreOffice)
both looked good, before and after the patch.

Importing the ISO-8859 CSV cut off the strings at the first non-ASCII
char. Importing the UTF-8 CSV worked as epected.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as expected, passes tests and QA script.
Comment 17 Galen Charlton 2014-03-14 15:21:46 UTC
Pushed to master.  Thanks, Tomás!
Comment 18 Fridolin Somers 2014-06-12 08:23:18 UTC
Pushed to 3.14.x, will be in 3.14.08
Comment 19 Kyle M Hall 2014-06-13 12:46:04 UTC
Pushed to 3.12.x, will be in 3.12.13