413121 - UTF-8 in templates are garbaged

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[A. Shimono [:himorin]]

Attachment: garbaged output sample

I just created some Japanese templates to test a patch for tip, and i've got garbaged output.

Comment 1

[Max Kanat-Alexander]

Okay. What version of Template Toolkit did you test with? 

Just as a note: Adding a BOM is not a good solution. There are other very good solutions to this problem available on mailing lists, or if you research CPAN or the internals of Template Toolkit. In fact, Template Toolkit is already supposed to handle this, from my understanding, but I'm pretty sure that we can *make* it handle it, if we have to. (Probably just make it open template files in ':utf8' mode.)

Comment 2

[A. Shimono [:himorin]]

(In reply to comment #1)
> Okay. What version of Template Toolkit did you test with? 

Checking for    Template-Toolkit (v2.15)   ok: found v2.19
So, i think we use the newest version.

Comment 3

[A. Shimono [:himorin]]

> Just as a note: Adding a BOM is not a good solution. There are other very good
> solutions to this problem available on mailing lists, or if you research CPAN
> or the internals of Template Toolkit. In fact, Template Toolkit is already
> supposed to handle this, from my understanding, but I'm pretty sure that we can
> *make* it handle it, if we have to. (Probably just make it open template files
> in ':utf8' mode.)

I think we can do such thing (opening template files in ':utf8' mode) with CPAN modules, but i've failed once.
I've tried with Template::Stash::ForceUTF8 or something which uses (applies?) Stash mechanizm.

# or,, we can do with setting utf8 to global PerlIO?

Comment 4

[Max Kanat-Alexander]

No, don't modify PerlIO.

You only have to make $template->process open files in UTF-8 mode. I read somewhere how it could be done online, but I'm not sure where. You just have to add code to Bugzilla::Template, most likely.

Comment 5

[Marc Schumann [:Wurblzap]]

I don't experience any such garbled output with templates containing UTF-8 encoded characters for some reason. Maybe I can help here in some way I don't know yet. I suppose we're not facing a Bugzilla problem.

Comment 6

[Marc Schumann [:Wurblzap]]

Ok, wait... Sorry, I must've clicked somewhere wrong. I *do* see garbled characters.

Comment 7

[Vitaly Fedrushkov]

Same problem was seen in Bugzilla-ru (https://bugzilla.mozilla-russia.org/show_bug.cgi?id=318 and https://bugzilla.mozilla-russia.org/show_bug.cgi?id=235).  The solution was to _remove_ BOMs from every template.

Also it became clear that some editors and diff/patch tools are incapable of detecting differences (or making changes) in BOM only.

Comment 8

[Marc Schumann [:Wurblzap]]

Attachment: Work in progress

This fixes the problem at hand. It introduces a new problem though; constants are no longer showing up in TT output. I haven't found out why yet. Does somebody have an idea?

Comment 9

[Max Kanat-Alexander]

Comment on attachment 301662 [details] [diff] [review]
Work in progress

Perhaps you need to shift off $self and pass the entire @_ to the SUPER, after unshifting $text back in?

Also, you don't need to use Encode, you can just use utf8::decode. (Without even having to "use utf8".)

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch

The trick was to use a different way to tell the Template Toolkit about our custom parser class. The previous patch followed Template's docs, this one follows Template::Config's docs. Seeing that this works now, I consider the previous patch not working to be a bug in the Template Toolkit. Oh well.

(In reply to comment #9)
> Perhaps you need to shift off $self and pass the entire @_ to the SUPER, after
> unshifting $text back in?

Hmmm, the docs mention one parameter only, but I think that's a good idea anyway, so I did so. It works without it, though.

> Also, you don't need to use Encode, you can just use utf8::decode. (Without
> even having to "use utf8".)

Ok, done.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 301802 [details] [diff] [review]
Patch

>+    utf8::decode($text) if Bugzilla->params->{'utf8'};

  Just to be safe I think we should also check "!utf8::is_utf8($text)".

>--- ../head/Bugzilla/Template.pm	2007-12-20 00:23:59.456261000 +0100
>+++ ./Bugzilla/Template.pm	2008-02-07 02:47:11.463965100 +0100
> [snip]
>+    # Make the Template Toolkit use our UTF-8-aware parser.
>+    $Template::Config::PARSER = 'Bugzilla::Template::Parser';

  This will affect other mod_perl apps running on the same server, which concerns me. There's really no way to set this on a per-object basis?

Comment 12

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2

(In reply to comment #11)
> (From update of attachment 301802 [details] [diff] [review])
> >+    utf8::decode($text) if Bugzilla->params->{'utf8'};
> 
>   Just to be safe I think we should also check "!utf8::is_utf8($text)".

Ok.

> >--- ../head/Bugzilla/Template.pm	2007-12-20 00:23:59.456261000 +0100
> >+++ ./Bugzilla/Template.pm	2008-02-07 02:47:11.463965100 +0100
> > [snip]
> >+    # Make the Template Toolkit use our UTF-8-aware parser.
> >+    $Template::Config::PARSER = 'Bugzilla::Template::Parser';
> 
>   This will affect other mod_perl apps running on the same server, which
> concerns me. There's really no way to set this on a per-object basis?

It turns out it's even more complicated -- as I understand it, Template::Config's preload() magic makes it load its helper modules on startup, when running under mod_perl. Thus, Template::Config package variables are unusable under mod_perl. Afaict my proposed solution using Template::Config package variables therefore does not work under mod_perl at all.

Oh well. I found a semi-not-too-messy way around all this, which is much closer to my original approach of attachment 301662 [details] [diff] [review]. I'm not altogether happy with it, but it works, and I can't think of a better way, so here goes.

It turns out that it is necessary to pass the config hash to the Parser constructor *after* the constants namespace has been set up. Furthermore, the Service constructor needs the Parser object to be used in its config hash. So the Parser object must be constructed in between. Unfortunately, both things happen in Template's original _init. So what I'm doing is override _init (which is documented with Template::Base, so I don't feel bad about it), let both things happen, create the Parser object, and re-create the Service object.

Comment 13

[Frédéric Buclin]

Comment on attachment 302359 [details] [diff] [review]
Patch 2

./Bugzilla/Template/Parser.pm

>+use Bugzilla;

Do not call Bugzilla from a .pm module. This confuses Bugzilla.

Comment 14

[Marc Schumann [:Wurblzap]]

Ok, gone in next patch or on checkin.

Comment 15

[Marc Schumann [:Wurblzap]]

For the record, this is a regression, probably of bug 363153.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 302359 [details] [diff] [review]
Patch 2

Yeah, if this works, it looks fine to me. Of course, I'd rather just shift off $self in _init and pass all of @_ to the SUPER method or other methods that require it, but if all these parameters are currently specified in TT's API docs, then this is technically OK.

Comment 17

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2-b

Alternative patch, passing @_ on to SUPER

Comment 18

[Max Kanat-Alexander]

Comment on attachment 303115 [details] [diff] [review]
Patch 2-b

In my copy of TT, _init returns $self, not 0 or 1.

I think you should just accept whatever SUPER::_init returns and return that.

Comment 19

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.0.1

Comment 20

[Max Kanat-Alexander]

Comment on attachment 303127 [details] [diff] [review]
Patch 2.0.1

Yeah, looks good to me. This doesn't have too much of a perf hit, does it?

Comment 21

[Marc Schumann [:Wurblzap]]

Can't say a thing about performance here... I'm on slow installs at all times.

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.87; previous revision: 1.86
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Parser.pm,v
done
Checking in Bugzilla/Template/Parser.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Parser.pm,v  <--  Parser.pm
initial revision: 1.1
done