Marc Schumann [:Wurblzap] Assignee
	Description • 18 years ago

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.

	Frédéric Buclin
	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.

	Max Kanat-Alexander
	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.

	Marc Schumann [:Wurblzap] 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.

	Marc Schumann [:Wurblzap] Assignee
	Comment 4 • 18 years ago

Heh. This removes nearly as much as it adds. Fun.

	Marc Schumann [:Wurblzap] Assignee
	Comment 5 • 18 years ago

Unrotted.

	Frédéric Buclin
	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).

	Frédéric Buclin
	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.

	Marc Schumann [:Wurblzap] Assignee
	Comment 8 • 17 years ago

Work in progress. With any luck, it'll work as-is as soon as bug 390756 is fixed.

	Max Kanat-Alexander
	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.

	Marc Schumann [:Wurblzap] 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?

	Max Kanat-Alexander
	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.

	Marc Schumann [:Wurblzap] 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.

	Frédéric Buclin
	Comment 13 • 17 years ago

Bugzilla::Config::Common::check_languages() should go away as it's not longer in use.

	Marc Schumann [:Wurblzap] Assignee
	Comment 14 • 17 years ago

Unrotted.
Thanks for comment 13 – I've removed check_languages now.

	Max Kanat-Alexander
	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.)

	Marc Schumann [:Wurblzap] Assignee
	Comment 16 • 17 years ago

Adressing comments.

	Marc Schumann [:Wurblzap] 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.

	Frédéric Buclin
	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.

	Marc Schumann [:Wurblzap] 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.

	Max Kanat-Alexander
	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.

	Frédéric Buclin
	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

	Frédéric Buclin
	Comment 22 • 17 years ago

a=me for 3.1.1 for the updated patch.

	Marc Schumann [:Wurblzap] Assignee
	Comment 23 • 17 years ago

Addressing comments (the caching stays though as agreed on IRC). Carrying forward r+.

	Marc Schumann [:Wurblzap] 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

	Max Kanat-Alexander
	Comment 25 • 16 years ago

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.