Bug 11302

Summary: Template::output should deal with object
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: abl, boutrosboutrosboutros, dpavlin, dubyk, josef.moravec, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11290
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10855
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11944
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 11302: Template::output should deal with objects
Bug 11302: Template::output should deal with objects

Description Jonathan Druart 2013-11-26 08:35:32 UTC
This report replaces bug 6554.
Comment 1 Jonathan Druart 2013-11-26 08:35:49 UTC
*** Bug 6554 has been marked as a duplicate of this bug. ***
Comment 2 Jonathan Druart 2013-11-26 08:41:05 UTC Comment hidden (obsolete)
Comment 3 Petter Goksøyr Åsen 2013-11-29 10:29:01 UTC
When I apply this patch, I get failure on the plugins page:
http://localhost:8080/cgi-bin/koha/plugins/plugins-home.pl

Software error:

Not a HASH reference at /home/boutros/Projects/deichman/kohaclone/C4/Templates.pm line 158.

Note that I have installed a plugin with non-ascii characters, so that might be why
Comment 4 Marcel de Rooy 2013-11-29 12:51:56 UTC
First observation: If I put a diacritical char in the branchname, the staff home page will show a mangled char in the upper-right corner (directly after login). If you refresh after login or come back there, it will disappear.
Comment 5 Marcel de Rooy 2013-11-29 12:57:10 UTC
Some (!) additional observations:
Facets in staff search show mangled chars.
Patron category (with latin diacritic and chinese char) on memberentry.pl mangled.
Branch name mangled too on memberentry.pl
Catalogue detail: mangled chars all over the place.

Failed QA
Comment 6 Jonathan Druart 2013-12-02 10:49:41 UTC
I was just a try. If it does not work, I reopen the initial report and close this one.
Comment 7 Jacek Ablewicz 2014-03-12 10:01:43 UTC
Created attachment 26151 [details] [review]
Bug 11302: Template::output should deal with objects

If a ref is not HASH or ARRAY, C4::Template::output assumes it
is a scalar. Which is wrong, it could be an object.

Note: this is yet another / alternative take on the problem,
with only bare minimum changes made to the code base, so
this (counter-)patch - while not pretty - hopefully should be
less regression-prone. Right now, the only white-listed object
this patch deals with is 'Koha::AdditionalField' introduced
by bug 10855. In case it tests OK, without any regressions, we can
add another objects (C4::Category etc.) later on.

Test plan:
1) apply bug 10855 first; observe that non-ascii characters
(in authorized category names or additional field labels) are not
properly handled,
2) apply patch,
3) ensure that encoding of non-ascii characters for features added
by 10855 got fixed,
4) there should be no character-encoding-related regressions
of any kind appearing anywhere in the system.
Comment 8 Dobrica Pavlinusic 2014-03-13 16:04:37 UTC
Sorry Jacek your simpler approach doesn't work for authorized values with UTF-8 characters in them (in fact, it doesn't even return double encoded utf-8, it just doesn't return anything which is somewhat puzzling since I can't figure out from code why it does so).
Comment 9 Jacek Ablewicz 2014-03-14 08:00:08 UTC
(In reply to Dobrica Pavlinusic from comment #8)
> Sorry Jacek your simpler approach doesn't work for authorized values with
> UTF-8 characters in them (in fact, it doesn't even return double encoded
> utf-8, it just doesn't return anything which is somewhat puzzling since I
> can't figure out from code why it does so).

It took me a while to reproduce this issue, but indeed there seems to be at least one serious problem with this particular approach, which is affecting bug 10855 in non intuitive way (I'm not entirely sure it's the same problem you've encountered - it can be yet another/new one as well). Anyway, it boils down to this:
- unpatched C4/Template.pm does not encode hash keys, and does not deal with objects at all
- patched C4/Template.pm does deal with whitelisted objects, but it only encodes scalar values within them (in the same exact manner as unpatched version deals with ordinary, unblessed hashes)
- if there are utf8 characters in additional field names, some code fragments introduced by 10855 - e.g. this one, used repeatedly in subscription-add.tt

   additional_fields.${field.name} 

wouldn't work any longer. While field.name (being a scalar value) gets encoded by patched C4/Template.pm, keys in additional_fields hash do not.

What's worse, I don't see any easy solution for this problem (other than making some - pretty substantial - changes to 10855). We can patch Template.pm somehow further, so it will encode hash keys - but that's rather risky move IMO; who knows how many regressions we may get elsewhere in the system..
Comment 10 Dobrica Pavlinusic 2014-03-14 08:12:04 UTC
I suspect that touching encoding in C4::Template::output is wrong direction to take.

I would rather decode utf-8 as soon as it arrives in Koha and have correct encoding within perl code and then let TT deal with encoding it back to binary bytes by using binmode( STDOUT, ":utf8" ); instead of traversing all hashes we are passing to TT which has performance impact.

I will extend http://wiki.koha-community.org/wiki/Handling_UTF8_in_development to include more information about it and prepare initial patch with replaces usage of utf8::(en|de)code witn Encode::(en|de)code so we can see if this approach is feasible.
Comment 11 Jacek Ablewicz 2014-03-14 11:35:38 UTC
(In reply to Dobrica Pavlinusic from comment #10)

> I will extend
> http://wiki.koha-community.org/wiki/Handling_UTF8_in_development to include
> more information about it and prepare initial patch with replaces usage of
> utf8::(en|de)code witn Encode::(en|de)code so we can see if this approach is
> feasible.

It would be really great to get rid of all those numerous ::[en|de]code, html_entity and so on.. Sounds like a pretty ambitious task, though. If I understand correctly, essentially it would be something like bug 6554 revival (which was expected for 3.14 but unfortunately got reverted), only better this time. Hopefully it will finally be included in 3.16 ;)
Comment 12 Jonathan Druart 2014-03-14 15:32:30 UTC
This patch does not fix correctly the issue. See bug 11944 for the next step.