Closed Bug 493242 Opened 15 years ago Closed 15 years ago

Garbaged UTF-8 characters on show_activity log

Categories

(Bugzilla :: Bugzilla-General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Bugzilla 3.4

People

(Reporter: himorin, Assigned: vitaly.fedrushkov)

References

Details

(Keywords: intl)

Attachments

(1 file, 2 obsolete files)

# 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.
Nope, can't reproduce on bugzilla-tip.
Status: UNCONFIRMED → RESOLVED
Closed: 15 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
(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?
(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.)
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
Keywords: l12y
Summary: Garbaged charactor on show_activity log → Garbaged UTF-8 characters on show_activity log
SnowyOwl: no, this is not l12y, but i18n.
Keywords: l12yintl
Attached patch A very straightforward patch (obsolete) — Splinter Review
Looks like we already don't need fieldnames, since bug 344513.
Assignee: general → vitaly.fedrushkov
Status: NEW → ASSIGNED
Attachment #405782 - Flags: review?(mkanat)
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 on attachment 405782 [details] [diff] [review] A very straightforward patch Looks right to me.
Attachment #405782 - Flags: review?(mkanat) → review+
Flags: approval3.4+
Flags: approval+
Target Milestone: --- → Bugzilla 3.4
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.
(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.
(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.
Attachment #405782 - Flags: review-
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.
Flags: approval3.4+
Flags: approval+
Attached patch Not so straightforward patch (obsolete) — Splinter Review
(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 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 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-
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+
Attached patch Patch v3Splinter Review
(In reply to comment #18) > Vitaly, can you update your patch, please? Done.
Attachment #407338 - Attachment is obsolete: true
Attachment #408303 - Flags: review?(LpSolit)
Attachment #408303 - Flags: review?(LpSolit) → review+
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.
Flags: approval3.4+
Flags: approval+
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
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: