UTF-8 in templates are garbaged

RESOLVED FIXED in Bugzilla 3.2

Status

()

defect
--
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: himorin, Assigned: Wurblzap)

Tracking

({intl, regression})

Bug Flags:
approval +
blocking3.2 +

Details

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

12 years ago
I just created some Japanese templates to test a patch for tip, and i've got garbaged output.
Flags: blocking3.2?
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.)
Severity: critical → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.2? → blocking3.2+
Target Milestone: --- → Bugzilla 3.2
Reporter

Comment 2

12 years ago
(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.
Reporter

Comment 3

12 years ago
> 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?
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.
Assignee

Comment 5

12 years ago
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.
Assignee

Comment 6

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

Comment 7

12 years ago
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.
Assignee

Comment 8

11 years ago
Posted patch Work in progress (obsolete) — Splinter Review
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?
Assignee: ui → wurblzap
Status: NEW → ASSIGNED
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".)
Assignee

Comment 10

11 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #301662 - Attachment is obsolete: true
Attachment #301802 - Flags: review?(mkanat)
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?
Attachment #301802 - Flags: review?(mkanat) → review-
Assignee

Comment 12

11 years ago
Posted patch Patch 2 (obsolete) — Splinter Review
(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.
Attachment #301802 - Attachment is obsolete: true
Attachment #302359 - Flags: review?(mkanat)

Comment 13

11 years ago
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.
Assignee

Comment 14

11 years ago
Ok, gone in next patch or on checkin.
Assignee

Comment 15

11 years ago
For the record, this is a regression, probably of bug 363153.
Keywords: regression
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.
Attachment #302359 - Flags: review?(mkanat) → review+

Updated

11 years ago
Flags: approval+
Assignee

Comment 17

11 years ago
Posted patch Patch 2-b (obsolete) — Splinter Review
Alternative patch, passing @_ on to SUPER
Attachment #303115 - Flags: review?(mkanat)
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.
Attachment #303115 - Flags: review?(mkanat) → review-
Assignee

Comment 19

11 years ago
Posted patch Patch 2.0.1Splinter Review
Attachment #302359 - Attachment is obsolete: true
Attachment #303115 - Attachment is obsolete: true
Attachment #303127 - Flags: review?(mkanat)
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?
Attachment #303127 - Flags: review?(mkanat) → review+
Assignee

Comment 21

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.