Garbaged UTF-8 characters on show_activity log

VERIFIED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Bugzilla-General
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: himorin, Assigned: Vitaly Fedrushkov)

Tracking

({intl})

Bugzilla 3.4
Bug Flags:
approval +
approval3.4 +
blocking3.4.3 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

4.48 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
# 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

9 years ago
Nope, can't reproduce on bugzilla-tip.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WORKSFORME
I can reproduce on my own (unmodified) tip, 3.4 and 3.2 installs using description 試験.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Version: 3.5 → 3.2

Comment 3

9 years ago
(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?
(Reporter)

Comment 4

9 years ago
(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;
(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.)
(Assignee)

Comment 6

9 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

9 years ago
Keywords: l12y
Summary: Garbaged charactor on show_activity log → Garbaged UTF-8 characters on show_activity log
(Reporter)

Comment 7

9 years ago
SnowyOwl: no, this is not l12y, but i18n.
Keywords: l12y → intl
(Assignee)

Comment 8

9 years ago
Created attachment 405782 [details] [diff] [review]
A very straightforward patch

Looks like we already don't need fieldnames, since bug 344513.
Assignee: general → vitaly.fedrushkov
Status: NEW → ASSIGNED
Attachment #405782 - Flags: review?(mkanat)
(Assignee)

Comment 9

9 years ago
For this to work on UTF-8 named custom fields, we need recent Template Toolkit version.
Depends on: 457524
Version: 3.2 → 3.5

Comment 10

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

Looks right to me.
Attachment #405782 - Flags: review?(mkanat) → review+

Updated

9 years ago
Flags: approval3.4+
Flags: approval+
Target Milestone: --- → Bugzilla 3.4
(Reporter)

Comment 11

9 years ago
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.
(Assignee)

Comment 12

9 years ago
(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

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

Updated

9 years ago
Attachment #405782 - Flags: review-

Comment 14

9 years ago
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.

Updated

9 years ago
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 15

9 years ago
Created attachment 407338 [details] [diff] [review]
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.
Attachment #405782 - Attachment is obsolete: true
Attachment #407338 - Flags: review?(LpSolit)

Comment 16

9 years ago
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

9 years ago
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.
Attachment #407338 - Flags: review?(LpSolit) → review-

Comment 18

9 years ago
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?
Flags: blocking3.4.3+
(Assignee)

Comment 19

9 years ago
Created attachment 408303 [details] [diff] [review]
Patch v3

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

Done.
Attachment #407338 - Attachment is obsolete: true
Attachment #408303 - Flags: review?(LpSolit)

Updated

9 years ago
Attachment #408303 - Flags: review?(LpSolit) → review+

Comment 20

9 years ago
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.

Updated

9 years ago
Flags: approval3.4+
Flags: approval+

Comment 21

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 22

9 years ago
(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.
(Assignee)

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.