Bug 8872

Summary: Improve encoding for xslt files, showmarc and opac-showmarc
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: I18N/L10NAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Mason James <mtj>
Severity: minor    
Priority: P5 - low CC: chris, f.demians, liz, mtj, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 20083    
Attachments: Patch 1 for Templates module
Patch 2 for intranet xslt
Patch 3 for opac xslt
Patch 4 for opac-showmarc
Patch 5 for staff showmarc
Bug 8872 Encoding issues in Templates.pm
Bug 8872: Add UTF-8 encoding to intranet xslt files
Bug 8872 Add UTF-8 encoding to opac xslt files
Bug 8872: Changes for opac-showmarc
Bug 8872: Changes for intranet showmarc
Simplifies template (eliminating intranet-bottom include). Makes encoding for card view more consistent with approach in Templates module. Rearranges a few lines in script for consistency and performance.
Bug 8872: Changes for intranet showmarc
Subject: [PATCH] Bug 8872: Changes for opac-showmarc
Bug 8872: Changes for opac-showmarc
Bug 8872: Changes for intranet showmarc
Bug 8872: Changes for intranet showmarc

Description Marcel de Rooy 2012-10-04 09:01:53 UTC

    
Comment 1 Marcel de Rooy 2012-10-10 13:53:26 UTC
I had some incidental cases where diacritical characters a.o. were wrongly encoded in Koha. This appeared to originate from the Template output routine. This routine encodes all variables before sending them out. But merging these values with other non-encoded variables [somewhere in the template process call] may occasionally let Perl make a wrong choice when encoding/decoding. The attached patches are proposed to resolve the situation and make Koha more consistent.

Patch 1
Adds specific encoding parameter when creating Template object.
Note that the instruction binmode => ':utf8' could have been added also to the 
process method. But since it outputs to a Perl variable, this is not needed.
Finally, instead of the old encoding of all parameters before processing, the 
output of process is encoded to UTF-8 octets. Note that Perl could have handled
this internally, but this would require adding a use open statement, setting IO
layers to UTF-8 in all scripts.

Patch 2
Add UTF-8 encoding to intranet xslt files in output clause.

Patch 3
Add UTF-8 encoding to opac xslt files also.
Does also fix head and body tags in MARC21slim2OPACMARCdetail.xsl.

Patch 4
Simplifies opac-showmarc template (eliminating opac-bottom include).
Makes encoding for card and html view more consistent with approach in 
Templates module.
Rearranges a few lines in script for consistency and performance.

Patch 5
Simplifies showmarc template (eliminating intranet-bottom include).
Makes encoding for card view more consistent with approach in Templates module.
Rearranges a few lines in script for consistency and performance.

All patches should be combined. Finding a test case may be quite hard. Please 
choose some records with special characters. And check in several opac places 
and in staff that they are displayed correctly. Especially check MARC 
view/plain MARC view in opac and MARC/Card view in intranet results. Also check 
normal view in XSLT mode and non-XSLT mode.
Comment 2 Marcel de Rooy 2012-10-10 13:55:33 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2012-10-10 13:57:04 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2012-10-10 13:57:54 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2012-10-10 13:59:03 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2012-10-10 14:00:02 UTC Comment hidden (obsolete)
Comment 7 Jared Camins-Esakov 2012-10-21 15:30:34 UTC Comment hidden (obsolete)
Comment 8 Jared Camins-Esakov 2012-10-21 15:30:40 UTC
Created attachment 12987 [details] [review]
Bug 8872: Add UTF-8 encoding to intranet xslt files

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 9 Jared Camins-Esakov 2012-10-21 15:30:43 UTC
Created attachment 12988 [details] [review]
Bug 8872 Add UTF-8 encoding to opac xslt files

Does also fix head and body tags in MARC21slim2OPACMARCdetail.xsl.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 10 Jared Camins-Esakov 2012-10-21 15:30:46 UTC Comment hidden (obsolete)
Comment 11 Jared Camins-Esakov 2012-10-21 15:30:48 UTC Comment hidden (obsolete)
Comment 12 Paul Poulain 2012-10-28 18:54:42 UTC
I'm a little bit reluctant to push this patc for 3.10 because it changes things in the core of Koha. If we miss something, it can have very bad side effect. Do people here agree to wait for after 3.10.0 release ?
Comment 13 Marcel de Rooy 2012-10-29 10:25:08 UTC
(In reply to comment #12)
> I'm a little bit reluctant to push this patc for 3.10 because it changes
> things in the core of Koha. If we miss something, it can have very bad side
> effect. Do people here agree to wait for after 3.10.0 release ?

Well, I do not mind holding this patch a little longer. Probably other patches touching core modules should wait now too. About the very bad side effect: We use this in production for some time already.

Note that it should still be QAed. Did you do that?
Comment 14 Mason James 2012-11-01 12:29:47 UTC Comment hidden (obsolete)
Comment 15 Mason James 2012-11-01 12:31:50 UTC Comment hidden (obsolete)
Comment 16 Mason James 2012-11-01 12:32:43 UTC
(In reply to comment #15)
> Created attachment 13172 [details] [review]
> Bug 8872: Changes for intranet showmarc
> 
> Simplifies template (eliminating intranet-bottom include). Makes encoding
> for card view more consistent with approach in Templates module. Rearranges
> a few lines in script for consistency and performance.
> 
> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>


i had a small merge conflict, so i rebased this patch against master
Comment 17 Marcel de Rooy 2012-11-01 12:35:58 UTC
Thanks for rebasing!
But I discovered now that this patch needs a small followup for opac facets and opac search results paging. Somehow diacritics are not handled consistently there yet..
Comment 18 Mason James 2012-11-01 12:37:24 UTC Comment hidden (obsolete)
Comment 19 Mason James 2012-11-01 12:43:05 UTC
(In reply to comment #18)
> Created attachment 13173 [details] [review]
> Subject: [PATCH] Bug 8872: Changes for opac-showmarc
> 
> Simplifies template (eliminating opac-bottom include).
> Makes encoding for card and html view more consistent with approach in
> Templates module.
> Rearranges a few lines in script for consistency and performance.
> 
> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>

meh, and a rebase for this one too...
Comment 20 Mason James 2012-11-01 12:58:09 UTC
(In reply to comment #17)
> Thanks for rebasing!
> But I discovered now that this patch needs a small followup for opac facets
> and opac search results paging. Somehow diacritics are not handled
> consistently there yet..

wow, great spotting! :)


Marcel, do you suggest we wait for a follow-up patch before QA-ing?

or, should we QA this patch-set 'as is' now?
Comment 21 Marcel de Rooy 2012-11-01 13:00:53 UTC
Please go ahead.
A followup will need additional signoff ;)
Comment 22 Marcel de Rooy 2012-11-01 13:46:37 UTC
Mason: Perhaps we should still wait a little. I keep finding some instances where input from outside (like in a url parameter) is not decoded when passing it to perl.
That should be the ideal way to approach it, but if we not cover all those instances these strings will be double encoded. Like opac facets coming from zebra render without decoding. Etc..
Comment 23 Marcel de Rooy 2012-11-05 08:35:10 UTC
Created attachment 13244 [details] [review]
Bug 8872: Changes for opac-showmarc
Comment 24 Marcel de Rooy 2012-11-05 08:35:46 UTC
Comment on attachment 12986 [details] [review]
Bug 8872 Encoding issues in Templates.pm

Marking the first patch (for Templates.pm) as obsolete.
Comment 25 Marcel de Rooy 2012-11-05 08:42:50 UTC
I have removed the patch for Templates.pm since this would require some more changes to Koha than planned for this report.
Koha should follow the principle: decode everything coming in from outside, encode everything when sending it to outside. But in several places this does not happen: especially for url parameters and template file contents.
The first patch did solve the template file contents but since it would not touch all places where url parameters are passed in, it is not fully working.

What remains in this report, is a fix for xslt files and opac-showmarc and showmarc. I think that in this form the patches could be pushed (after QA).

I will ask Jared to confirm since he signed off on the full set of five patches.
Comment 26 Marcel de Rooy 2012-11-05 08:50:35 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2012-11-05 11:07:09 UTC
Created attachment 13248 [details] [review]
Bug 8872: Changes for intranet showmarc

Changing one line only: adding empty string to remove uninit-warning from log.
Comment 28 Jared Camins-Esakov 2012-11-05 16:10:13 UTC
(In reply to comment #25)
> I have removed the patch for Templates.pm since this would require some more
> changes to Koha than planned for this report.
> Koha should follow the principle: decode everything coming in from outside,
> encode everything when sending it to outside. But in several places this
> does not happen: especially for url parameters and template file contents.
> The first patch did solve the template file contents but since it would not
> touch all places where url parameters are passed in, it is not fully working.
> 
> What remains in this report, is a fix for xslt files and opac-showmarc and
> showmarc. I think that in this form the patches could be pushed (after QA).
> 
> I will ask Jared to confirm since he signed off on the full set of five
> patches.

Agreed.
Comment 29 Mason James 2012-11-06 04:55:26 UTC
(In reply to comment #21)
> Please go ahead.
> A followup will need additional signoff ;)

tested OPAC and staff with no regressions, passing QA...



mason@xen1:~/g/head$ koha-qa.pl -c 4
testing 4 commit(s) (applied to 62ce0ba 'Merge branch 'bug_8842' into 3.12-ma')

 d86e82b 8872 Changes for intranet showmarc
 - catalogue/showmarc.pl
 - koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/showmarc.tt

 03ac92c 8872 Changes for opac-showmarc
 - koha-tmpl/opac-tmpl/prog/en/modules/opac-showmarc.tt
 - opac/opac-showmarc.pl

 7b0279c Bug 8872 Add UTF-8 encoding to opac xslt files
 - koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACDetail.xsl
 - koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACMARCdetail.xsl
 - koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl
 - koha-tmpl/opac-tmpl/prog/en/xslt/NORMARCslim2OPACDetail.xsl
 - koha-tmpl/opac-tmpl/prog/en/xslt/NORMARCslim2OPACResults.xsl
 - koha-tmpl/opac-tmpl/prog/en/xslt/UNIMARCslim2OPACDetail.xsl
 - koha-tmpl/opac-tmpl/prog/en/xslt/UNIMARCslim2OPACResults.xsl

 b373dbc Bug 8872: Add UTF-8 encoding to intranet xslt files
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2ATOM.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2DC.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2MODS.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2OAIDC.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2RDFDC.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2RDFDC.xsl.1
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2RSS2.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetResults.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim9522852.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2OAIDC.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetDetail.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARC_compact.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2OAIDC.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2intranetDetail.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2intranetResults.xsl
 - koha-tmpl/intranet-tmpl/prog/en/xslt/oilsMARC21slim2HTML.xsl

 * catalogue/showmarc.pl                                                    OK
 * opac/opac-showmarc.pl                                                    OK
 * koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/showmarc.tt            OK
 * koha-tmpl/opac-tmpl/prog/en/modules/opac-showmarc.tt                     OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACDetail.xsl               OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACMARCdetail.xsl           OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl              OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/NORMARCslim2OPACDetail.xsl              OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/NORMARCslim2OPACResults.xsl             OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/UNIMARCslim2OPACDetail.xsl              OK
 * koha-tmpl/opac-tmpl/prog/en/xslt/UNIMARCslim2OPACResults.xsl             OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2ATOM.xsl                 OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2DC.xsl                   OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2MODS.xsl                 OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2OAIDC.xsl                OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2RDFDC.xsl                OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2RSS2.xsl                 OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl       OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetResults.xsl      OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim9522852.xsl               OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2OAIDC.xsl               OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetDetail.xsl      OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl     OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARC_compact.xsl                 OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2OAIDC.xsl               OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2intranetDetail.xsl      OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2intranetResults.xsl     OK
 * koha-tmpl/intranet-tmpl/prog/en/xslt/oilsMARC21slim2HTML.xsl             OK
Comment 30 Jared Camins-Esakov 2012-11-06 12:30:42 UTC
This patch has been pushed to master.
Comment 31 Paul Poulain 2012-11-06 17:10:42 UTC
Patch pushed to branch 3.10.x
(I much more prefer those ones than the previous ones -from side-effect risk POV- !)
Comment 32 Chris Cormack 2012-11-08 21:10:53 UTC
Pushed to 3.8.x, will be in 3.8.7
Comment 33 Liz Rea 2012-11-21 22:32:16 UTC
Patches do not apply to 3.6.x, please reopen and submit a patch for 3.6.x if necessary. 

Thanks!