Bug 8872 - Improve encoding for xslt files, showmarc and opac-showmarc
Summary: Improve encoding for xslt files, showmarc and opac-showmarc
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks: 20083
  Show dependency treegraph
 
Reported: 2012-10-04 09:01 UTC by Marcel de Rooy
Modified: 2018-01-30 17:46 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch 1 for Templates module (3.27 KB, patch)
2012-10-10 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch 2 for intranet xslt (14.91 KB, patch)
2012-10-10 13:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch 3 for opac xslt (5.99 KB, patch)
2012-10-10 13:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch 4 for opac-showmarc (3.90 KB, patch)
2012-10-10 13:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch 5 for staff showmarc (4.37 KB, patch)
2012-10-10 14:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8872 Encoding issues in Templates.pm (3.29 KB, patch)
2012-10-21 15:30 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8872: Add UTF-8 encoding to intranet xslt files (14.93 KB, patch)
2012-10-21 15:30 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8872 Add UTF-8 encoding to opac xslt files (6.01 KB, patch)
2012-10-21 15:30 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8872: Changes for opac-showmarc (3.93 KB, patch)
2012-10-21 15:30 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8872: Changes for intranet showmarc (4.40 KB, patch)
2012-10-21 15:30 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
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. (1.30 KB, patch)
2012-11-01 12:29 UTC, Mason James
Details | Diff | Splinter Review
Bug 8872: Changes for intranet showmarc (1.34 KB, patch)
2012-11-01 12:31 UTC, Mason James
Details | Diff | Splinter Review
Subject: [PATCH] Bug 8872: Changes for opac-showmarc (4.04 KB, patch)
2012-11-01 12:37 UTC, Mason James
Details | Diff | Splinter Review
Bug 8872: Changes for opac-showmarc (3.94 KB, patch)
2012-11-05 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8872: Changes for intranet showmarc (4.37 KB, patch)
2012-11-05 08:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8872: Changes for intranet showmarc (4.38 KB, patch)
2012-11-05 11:07 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 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!