493242 - Garbaged UTF-8 characters on show_activity log

Status: VERIFIED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[A. Shimono [:himorin]]

# Can someone confirm this?
See our test site for trunk.
http://bug-ja-trunk.test.mozilla.gr.jp/show_activity.cgi?id=4

Custom field with Japanese name are displayed in garbaged.
Other than show_activity page are ok, such as show_bug etc.

Comment 1

[Max Kanat-Alexander]

Nope, can't reproduce on bugzilla-tip.

Comment 2

[Teemu Mannermaa (:wicked)]

I can reproduce on my own (unmodified) tip, 3.4 and 3.2 installs using description 試験.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> I can reproduce on my own (unmodified) tip, 3.4 and 3.2 installs using
> description 試験.

  But can you reproduce it on landfill?

Comment 4

[A. Shimono [:himorin]]

(In reply to comment #3)
> > I can reproduce on my own (unmodified) tip, 3.4 and 3.2 installs using
> > description 試験.
> 
>   But can you reproduce it on landfill?

I'm not sure but it might depend on the perl moduel version??

BTW, the garbaged charactor seems to be illegaly encoded by perl utf-8 function.
And, I could fix with inserting utf8::decode in Bugzilla/Bugs.pm (as the followings).

Index: Bugzilla/Bug.pm
===================================================================
--- Bugzilla/Bug.pm     (revision 391)
+++ Bugzilla/Bug.pm     (working copy)
@@ -3192,6 +3192,7 @@
             $operation->{'who'} = $who;
             $operation->{'when'} = $when;
 
+utf8::decode($field);
             $change{'field'} = $field;
             $change{'fieldname'} = $fieldname;
             $change{'attachid'} = $attachid;

Comment 5

[Teemu Mannermaa (:wicked)]

(In reply to comment #3)
>   But can you reproduce it on landfill?

My test installs ARE on landfill. You can see my test results in wtip, w34 and w32 installs by looking at activity of bug #4910 on all of them. Could this be somehow mod_perl related since only bugzilla-tip install uses it?

Note that wtip if slightly more broken at the moment since I tried to use example chars as the name of custom field and that obviously is not supported at all. We should probably yell at creation time if there's an UTF8 name since one can't even simply delete/change the custom field name to fix this. And checksetup doesn't seem to be able to delete such column with bz_drop_column either since UTF8 chars seems to confuse it. (Dropping a column is only way to fix bz_schema since it can't be reinitialized/modified directly like fielddefs and bugs tables can be.)

Comment 6

[Vitaly Fedrushkov]

Confirmed independently by Bugzilla-ru users:
https://bugzilla.mozilla-russia.org/show_bug.cgi?id=608

Both bug/activity/table.html.tmpl  and bug/process/midair.html.tmpl are affected.

For English templates, symptom can be seen for a custom field with UTF-8 name.

For localized templates, all strings from field_descs are shown as bytes.

Comment 7

[A. Shimono [:himorin]]

SnowyOwl: no, this is not l12y, but i18n.

Comment 8

[Vitaly Fedrushkov]

Attachment: A very straightforward patch

Looks like we already don't need fieldnames, since bug 344513.

Comment 9

[Vitaly Fedrushkov]

For this to work on UTF-8 named custom fields, we need recent Template Toolkit version.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 405782 [details] [diff] [review]
A very straightforward patch

Looks right to me.

Comment 11

[A. Shimono [:himorin]]

Wait. This patch forces site admins to edit their every templates when they define new custom fields.
We must add some description about this to the administation page of custom fields.

And, is this dependence ok per the code policy?
As a site admin, I cannot accept such dependence.

Comment 12

[Vitaly Fedrushkov]

(In reply to comment #11)
> Wait. This patch forces site admins to edit their every templates when they
> define new custom fields.
> We must add some description about this to the administation page of custom
> fields.

You already have to, if you want to see your custom field names localized.

If not, field_descs has been already filled with description values from database...

For sure I prefer to have this patch tested in more instances before we commit.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #11)
> Wait. This patch forces site admins to edit their every templates when they
> define new custom fields.

  No, it doesn't. field_descs is populated with descriptions from the database when values aren't set manually.

Comment 14

[Frédéric Buclin]

Comment on attachment 405782 [details] [diff] [review]
A very straightforward patch

>-              [%+ change.field %]

It must be removed from filterexceptions.pl


>+              [%+ field_descs.${change.fieldname} %]

It must be HTML-filtered.

not ok 38 - (en/default) template/en/default/bug/activity/table.html.tmpl has unfiltered directives:
#   75:+ field_descs.${change.fieldname}
# --ERROR

Also, did you make sure that change.field is still in use somewhere? If not, you should update Bugzilla::Bug::GetBugActivity() accordingly (on tip only), which would make the SQL query easier as COALESCE() could go away.

Comment 15

[Vitaly Fedrushkov]

Attachment: Not so straightforward patch

(In reply to comment #14)
> >-              [%+ change.field %]
> It must be removed from filterexceptions.pl

Done

> >+              [%+ field_descs.${change.fieldname} %]
> It must be HTML-filtered.

Done

> Also, did you make sure that change.field is still in use somewhere? If not,
> you should update Bugzilla::Bug::GetBugActivity() accordingly (on tip only),
> which would make the SQL query easier as COALESCE() could go away.

Good question, and you are absolutely right: no usage besides bug/activity/show.html.tmpl (3 cases in total).  Removing.

Comment 16

[Frédéric Buclin]

Comment on attachment 407338 [details] [diff] [review]
Not so straightforward patch

>Index: Bugzilla/Bug.pm

>-    my $query = "
>-        SELECT COALESCE(fielddefs.description, " 
>+    my $query = "fielddefs.name, bugs_activity.attach_id, " .

Obviously, you didn't test your patch, else you would note that SELECT is missing. :-D

Comment 17

[Frédéric Buclin]

Comment on attachment 407338 [details] [diff] [review]
Not so straightforward patch

>Index: Bugzilla/Bug.pm

>+    my $query = "fielddefs.name, bugs_activity.attach_id, " .

Missing SELECT at the start of the SQL query.


You still have to remove obsolete code from Bugzilla/WebService/Bug.pm:

 # This is going to go away in the future from GetBugActivity
 # so we shouldn't put it in the API.
 delete $change->{field};


Otherwise looks good.

Comment 18

[Frédéric Buclin]

Not really a blocker, but it's so easy to fix the two remaining comments of my review that it would be a pity to not have it for 3.5.1 and 3.4.3.

Vitaly, can you update your patch, please?

Comment 19

[Vitaly Fedrushkov]

Attachment: Patch v3

(In reply to comment #18)
> Vitaly, can you update your patch, please?

Done.

Comment 20

[Frédéric Buclin]

Comment on attachment 408303 [details] [diff] [review]
Patch v3

Thanks for the patch! r=LpSolit

On the 3.4 branch, I will leave GetBugActivity() unmodified in case something (e.g. an extension) still uses change->field.

Comment 21

[Frédéric Buclin]

mkanat, do you want to update the release notes to include this fix?

tip:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.293; previous revision: 1.292
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.44; previous revision: 1.43
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.131; previous revision: 1.130
done
Checking in template/en/default/bug/activity/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/activity/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.19; previous revision: 1.18
done


3.4.2:

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.127.2.1; previous revision: 1.127
done
Checking in template/en/default/bug/activity/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/activity/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.17.2.1; previous revision: 1.17
done

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> mkanat, do you want to update the release notes to include this fix?

  No, I think it's just a nice addition that people will notice when they use it, not something that enough people would have noticed as a significant issue that they would want to upgrade just because of it, I think.