Closed
Bug 365378
Opened 18 years ago
Closed 17 years ago
The 'languages' parameter is not necessary
Categories
(Bugzilla :: Administration, task)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: Wurblzap, Assigned: Wurblzap)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
Wurblzap
:
review+
|
Details | Diff | Splinter Review |
The list of legal values of Bugzilla's "lang" user setting is initialized with the languages specified in the "language" param. All language packs installed afterwards are not being recognized. As to not rendering the setting worthless, this requires admins to manually issue SQL commands to add languages to the list of legal languages. Bugzilla should dynamically allow all installed languages.
Attachment #249959 -
Flags: review?
![]() |
||
Comment 1•18 years ago
|
||
Comment on attachment 249959 [details] [diff] [review] Patch editparams.cgi already update the 'lang' param itself, see bug 297186 comment 13. So this fix is useless.
Attachment #249959 -
Flags: review? → review-
![]() |
||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME
Target Milestone: Bugzilla 3.0 → ---
Comment 2•18 years ago
|
||
We have agreed in IRC to re-open this as an enhnacement with a new summary. The languages parameter can go away entirely, and we can just glob the template/ directory. We'll have to relnote this when it's fixed.
Severity: major → enhancement
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: User setting 'lang' fails to recognize language packs added after initial installation → The 'languages' parameter is not necessary
Target Milestone: --- → Bugzilla 3.2
Assignee | ||
Comment 3•18 years ago
|
||
In addition to comment 2, which is about the languages param, this bug is meant to remove the db-based value list of the 'lang' user setting (which is currently being updated with every change of the languages param) and to replace it with a dynamic value list.
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 4•18 years ago
|
||
Heh. This removes nearly as much as it adds. Fun.
Attachment #249959 -
Attachment is obsolete: true
Attachment #261841 -
Flags: review?(LpSolit)
Assignee | ||
Comment 5•18 years ago
|
||
Unrotted.
Attachment #261841 -
Attachment is obsolete: true
Attachment #263877 -
Flags: review?(LpSolit)
Attachment #261841 -
Flags: review?(LpSolit)
![]() |
||
Comment 6•18 years ago
|
||
Comment on attachment 263877 [details] [diff] [review] Patch 1.2 >+++ ./Bugzilla/Install.pm 2007-01-01 22:40:54.247574400 +0100 >+ lang => { subclass => 'Lang', > default => Bugzilla->params->{'defaultlanguage'} How is it supposed to work as 'defaultlanguage' no longer exists?? All occurences of 'defaultlanguage' should be removed (mkanat should have done this in his patch, in bug 374331).
![]() |
||
Comment 7•18 years ago
|
||
Comment on attachment 263877 [details] [diff] [review] Patch 1.2 Marc please update your patch as per our discussion on IRC.
Attachment #263877 -
Flags: review?(LpSolit)
Assignee | ||
Comment 8•17 years ago
|
||
Work in progress. With any luck, it'll work as-is as soon as bug 390756 is fixed.
Attachment #263877 -
Attachment is obsolete: true
Comment 9•17 years ago
|
||
Comment on attachment 275234 [details] [diff] [review] Patch 1.3 Since we use it like a normal constant and not like a subroutine, I think it's a bit confusing for bz_languages to be lowercase. Also, any particular reason it can't be in Bugzilla.pm (Bugzilla->languages)? If it can, we could cache it, and that would help.
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9) > (From update of attachment 275234 [details] [diff] [review]) > Since we use it like a normal constant and not like a subroutine, I think it's > a bit confusing for bz_languages to be lowercase. I followed the lead of bz_locations here. I don't mind either way as long as we're consistent. If we agree to make this upper case, I'll convert bz_locations to upper case as well. > Also, any particular reason it can't be in Bugzilla.pm (Bugzilla->languages)? > If it can, we could cache it, and that would help. Again, I followed the lead of bz_locations. It should be technically possible to cache the result of bz_locations and bz_languages, right? Or am I missing some mod_perl issue?
Comment 11•17 years ago
|
||
(In reply to comment #10) > I followed the lead of bz_locations here. I don't mind either way as long as > we're consistent. If we agree to make this upper case, I'll convert > bz_locations to upper case as well. No, bz_locations is fine as it is, because we *have* to call that like a subroutine ("bz_locations()") because of Perl syntax with hashrefs. > Again, I followed the lead of bz_locations. It should be technically possible > to cache the result of bz_locations and bz_languages, right? Or am I missing > some mod_perl issue? bz_locations has to be used during installation, before Bugzilla.pm can be loaded. If "languages" also has to be used in such a case, it will have to be a constant. Otherwise it can go into Bugzilla.pm.
Assignee | ||
Comment 12•17 years ago
|
||
Addressing comments. Bugzilla->languages is called by checksetup.pl, but it works fine when installing from scratch as well as when upgrading.
Attachment #275234 -
Attachment is obsolete: true
Attachment #277381 -
Flags: review?(mkanat)
![]() |
||
Comment 13•17 years ago
|
||
Bugzilla::Config::Common::check_languages() should go away as it's not longer in use.
Assignee | ||
Comment 14•17 years ago
|
||
Unrotted. Thanks for comment 13 – I've removed check_languages now.
Attachment #277381 -
Attachment is obsolete: true
Attachment #277448 -
Flags: review?(LpSolit)
Attachment #277381 -
Flags: review?(mkanat)
Comment 15•17 years ago
|
||
Comment on attachment 277448 [details] [diff] [review] Patch 2.1 I can confirm that the way it's used in checksetup is fine. Don't use $_ in the loop in Bugzilla.pm. Also, if we could avoid wantarray, that would be nice, too. (It makes the behavior of functions confusing.)
Assignee | ||
Comment 16•17 years ago
|
||
Adressing comments.
Attachment #277448 -
Attachment is obsolete: true
Attachment #277496 -
Flags: review?(mkanat)
Attachment #277448 -
Flags: review?(LpSolit)
Assignee | ||
Comment 17•17 years ago
|
||
Comment on attachment 277496 [details] [diff] [review] Patch 2.2 Asking Frédéric as well, as both of you have already taken a look at this bug's patches before.
Attachment #277496 -
Flags: review?(LpSolit)
![]() |
||
Comment 18•17 years ago
|
||
Comment on attachment 277496 [details] [diff] [review] Patch 2.2 >+++ ./Bugzilla/Install/DB.pm 2007-08-21 00:06:30.841048700 +0200 >+sub _make_lang_setting_dynamic { >+ my $dbh = Bugzilla->dbh; >+ # 2007-08-21 wurblzap@gmail.com - Bug 365378 >+ if ($dbh->selectrow_array(q{SELECT COUNT(*) FROM setting >+ WHERE name='lang' AND subclass IS NULL}, >+ undef)) { Nit: 'undef' is useless as no other argument is passed; this would make the SQL query a bit easier to read. Nit 2: also, I'm not a fan of these SQL queries being injected in the IF condition directly. I had to re-read it twice to realize that the SQL query was within an IF condition. And these additional brackets coming from q{} do not help in making it clearer. my $count = $dbh->...; if ($count) { ... } is much more readable, IMO. >+++ ./Bugzilla/Template/Plugin/Hook.pm 2007-08-21 07:45:33.159067200 +0200 >- my $languages = trim(Bugzilla->params->{'languages'}); >+ my $languages = join(',', Bugzilla->languages); I don't get it. Bugzilla->languages returns an arrayref, not an array. >+++ ./Bugzilla/Template.pm 2007-08-20 23:36:57.449336600 +0200 >- use_languages => [split(/[\s,]+/, Bugzilla->params->{'languages'})], >+ use_languages => [Bugzilla->languages], Same comment here. It's already an arrayref. >+++ ./Bugzilla/User/Setting/Lang.pm 2007-08-20 14:24:30.250421000 +0200 >+ return $self->{'legal_values'} = [Bugzilla->languages]; Same comment again. >+++ ./Bugzilla.pm 2007-08-21 07:49:42.597742400 +0200 >+ request_cache()->{languages} && return @{request_cache()->{languages}}; Nit: this usage of && is hard to understand, IMO. It looks like a test without really being one (as there is no IF). I much prefer the |return $foo if $foo| notation. >+ return request_cache()->{languages} = \@languages; As said above, you return an arrayref.
Assignee | ||
Comment 19•17 years ago
|
||
(In reply to comment #18) > Nit: > Nit 2: Modified accordingly. > I don't get it. Bugzilla->languages returns an arrayref, not an array. This was an error. It was intended to return an array (see POD). The initial call erroneously returned an arrayref, all following calls took the arrayref from the cache and correctly returned an array. (That's why I didn't notice anything when testing.) I reworked it so that it returns an arrayref now by design. > Nit: this usage of && is hard to understand, IMO. It looks like a test without > really being one (as there is no IF). I much prefer the |return $foo if $foo| > notation. Ok.
Attachment #277496 -
Attachment is obsolete: true
Attachment #277523 -
Flags: review?(LpSolit)
Attachment #277496 -
Flags: review?(mkanat)
Attachment #277496 -
Flags: review?(LpSolit)
Assignee | ||
Updated•17 years ago
|
Attachment #277523 -
Flags: review?(mkanat)
Comment 20•17 years ago
|
||
By the way, "SELECT 1" is better than "SELECT COUNT(*)" for what you're doing (just checking if any rows exist that match), but it doesn't make a huge difference. That's just a nit, not a big deal.
![]() |
||
Comment 21•17 years ago
|
||
Comment on attachment 277523 [details] [diff] [review] Patch 2.3 >+++ ./Bugzilla/Install/DB.pm 2007-08-21 14:18:32.421521000 +0200 >+sub _make_lang_setting_dynamic { >+ my $dbh = Bugzilla->dbh; >+ # 2007-08-21 wurblzap@gmail.com - Bug 365378 This comment should be moved right before the caller. >+ my $count = $dbh->selectrow_array(q{SELECT COUNT(*) FROM setting >+ WHERE name='lang' >+ AND subclass IS NULL}); >+ if ($count) { >+ $dbh->do(q{UPDATE setting SET subclass='Lang' WHERE name='lang'}); >+ $dbh->do(q{DELETE FROM setting_value WHERE name='lang'}); >+ } We usually add whitespaces around =; do it here as well. >+++ ./Bugzilla/User/Setting/Lang.pm 2007-08-21 14:24:59.715249800 +0200 >+ return $self->{'legal_values'} if defined $self->{'legal_values'}; Not sure it makes sense to cache it as it's already cached in Bugzilla->languages. This makes this line useless. >+=head1 SYNOPSIS >+ ./runtests.pl -v 11 throws: *** WARNING: empty section in previous paragraph at line 47 in file Bugzilla/User/Setting/Lang.pm This section should be removed. >+++ ./Bugzilla.pm 2007-08-21 14:21:42.793872800 +0200 >+ my @files = glob(bz_locations->{'templatedir'} . '/*'); Why not using catdir() to only select directories, as you did in Skin.pm? >+ # It's a language directory only if it contains "default" or >+ # "custom" >+ next unless (-d "$dir_entry/default" or -d "$dir_entry/custom"); Nit: maybe should you add in the comment that this will exclude CVS/ as well. I had to re-read the routine to understand why CVS/ was magically excluded despite it's a valid language per RFC 1766. :) Nit2: we usually use || instead of or. It would be good to change for consistency in our code. Your patch is working fine. Carry forward my r+ on an updated patch. r=LpSolit
Attachment #277523 -
Flags: review?(mkanat)
Attachment #277523 -
Flags: review?(LpSolit)
Attachment #277523 -
Flags: review+
![]() |
||
Comment 22•17 years ago
|
||
a=me for 3.1.1 for the updated patch.
Assignee | ||
Comment 23•17 years ago
|
||
Addressing comments (the caching stays though as agreed on IRC). Carrying forward r+.
Attachment #277523 -
Attachment is obsolete: true
Attachment #277607 -
Flags: review+
Assignee | ||
Comment 24•17 years ago
|
||
Thanks for the review, Frédéric. Tip: Checking in Bugzilla.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v <-- Bugzilla.pm new revision: 1.59; previous revision: 1.58 done Checking in editparams.cgi; /cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v <-- editparams.cgi new revision: 1.46; previous revision: 1.45 done Checking in Bugzilla/Install.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install.pm,v <-- Install.pm new revision: 1.16; previous revision: 1.15 done Checking in Bugzilla/Template.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v <-- Template.pm new revision: 1.75; previous revision: 1.74 done Checking in Bugzilla/Config/Common.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v <-- Common.pm new revision: 1.18; previous revision: 1.17 done Removing Bugzilla/Config/L10n.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/L10n.pm,v <-- L10n.pm new revision: delete; previous revision: 1.3 done Checking in Bugzilla/Install/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v <-- DB.pm new revision: 1.41; previous revision: 1.40 done Checking in Bugzilla/Template/Plugin/Hook.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v <-- Hook.pm new revision: 1.9; previous revision: 1.8 done RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting/Lang.pm,v done Checking in Bugzilla/User/Setting/Lang.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting/Lang.pm,v <-- Lang.pm initial revision: 1.1 done Checking in docs/xml/customization.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v <-- customization.xml new revision: 1.42; previous revision: 1.41 done Removing template/en/default/admin/params/l10n.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/l10n.html.tmpl,v <-- l10n.html.tmpl new revision: delete; previous revision: 1.3 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → FIXED
Comment 25•16 years ago
|
||
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•