Closed
Bug 493242
Opened 15 years ago
Closed 15 years ago
Garbaged UTF-8 characters on show_activity log
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
VERIFIED
FIXED
Bugzilla 3.4
People
(Reporter: himorin, Assigned: vitaly.fedrushkov)
References
Details
(Keywords: intl)
Attachments
(1 file, 2 obsolete files)
4.48 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
# 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•15 years ago
|
||
Nope, can't reproduce on bugzilla-tip.
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → WORKSFORME
Comment 2•15 years ago
|
||
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•15 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•15 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;
Comment 5•15 years ago
|
||
(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•15 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•15 years ago
|
Keywords: l12y
Summary: Garbaged charactor on show_activity log → Garbaged UTF-8 characters on show_activity log
Reporter | ||
Comment 7•15 years ago
|
||
SnowyOwl: no, this is not l12y, but i18n.
Assignee | ||
Comment 8•15 years ago
|
||
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•15 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•15 years ago
|
||
Comment on attachment 405782 [details] [diff] [review]
A very straightforward patch
Looks right to me.
Attachment #405782 -
Flags: review?(mkanat) → review+
Updated•15 years ago
|
Flags: approval3.4+
Flags: approval+
Target Milestone: --- → Bugzilla 3.4
Reporter | ||
Comment 11•15 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•15 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•15 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•15 years ago
|
Attachment #405782 -
Flags: review-
Comment 14•15 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•15 years ago
|
Flags: approval3.4+
Flags: approval+
Assignee | ||
Comment 15•15 years ago
|
||
(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•15 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•15 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•15 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•15 years ago
|
||
(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•15 years ago
|
Attachment #408303 -
Flags: review?(LpSolit) → review+
Comment 20•15 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•15 years ago
|
Flags: approval3.4+
Flags: approval+
Comment 21•15 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
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 22•15 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•15 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•