The languages param is a comma-separated list of the language packs you wish to use on your Bugzilla site. In order to list something there, you have to have a directory by that name within the template directory. Problem is, there is no validation check to ensure that the directory names you enter in that param actually exist. And if you fail to list "en" and you don't have anything else there, it blindly sets it, and then you can't get back in to fix it unless you have shell access. This is a major problem if you have someone with admin rights that doesn't have shell access.

Perhaps we should change how this works? Instead of explicitly listing "en", there's an implicit "en" on the end of whatever's there. So, by default, the field is empty, and you list additional language packs there. If you want en to have precedence over some of them, you just list it again, in the middle. Hmm. Would that be more complicated for the admin to understand? Gerv

defparams has a mechanism for validating params. We just need a validation routine which verifies that the directories you listed actually exist. Pretty basic I'd think.

well, we'd need to check that they have all hte correct templates too, which is harder. Just append en, I think.

what if they remove en or they don't want it used? I thought that was the whole point of the param. Checking that the directories exist should be enough. If they install a bad template set that's the sysadmin's problem. The problem trying to be prevented here is to keep someone who does have access to tweakparams but doesn't have shell access on the machine from screwing it up.

I'm going off that idea, actually. We should just check that en appears in the list somewhere, and assume that they haven't been stupid enough to delete some of the important default templates (header, footer etc.). Gerv

what if they don't want 'en'? Suicide on a cvs install, but perfectly reasonable on a stable release with a fully-localized template set available.

So perhaps we need to check for the existence of those templates which are required to get a copy of editparams.cgi working. At the moment, that's the header and footer, and their dependencies. Hmm - still a fairly long list. > what if they don't want 'en'? If they have a fully localised set available, there's no harm in having "en" as well. It's not like deleting the templates will save disk space... Gerv

But having it available in that list allows a client browser to specify that they want English and get it. If the admin wants to customize their site, and the site is Spanish, and they don't want to duplicate their work, they might disable the English templates and only use the Spanish ones. If they're forced to leave English in the list then a user can still get to the English templates, even though they might be lacking the customizations they did.

Yeah, OK... you're right :-) Let's just check that the directories exist. Gerv

Mine.

Could you also change editparams.cgi to say, in the description for this param: "Available languages are en, de, fr, ..."? Gerv

Comment on attachment 130008 [details] [diff] [review] v2: Increase usebility by showing the languages available. > "Available languages are en, de, fr, ..."? | + 'Available languages: ' . find_languages() , I think I should change this to the proposed "Available languages are ".find_languages."." But I'm first waiting for other nits... (A "and" would be nice: 'de, en and fr', but I think this is not that easy and thus probably not worth doing so for editparams.cgi.)

+ my $deft_path = File::Spec->catdir('template', $lang, 'custom'); + my $cust_path = File::Spec->catdir('template', $lang, 'custom'); Should the first of these lines end with: | 'default'); | ?

Comment on attachment 130008 [details] [diff] [review] v2: Increase usebility by showing the languages available. >>+sub check_languages { >+ my @languages = split /,/, trim($_); >+ if(!scalar(@languages)) { >+ return "You need to specify a language tag." This isn't localisable - but I believe the other functions have this problem, so never mind. >+ } >+ foreach my $language (@languages) { >+ if( ! -d 'template/'.trim($language).'/custom' >+ && ! -d 'template/'.trim($language).'/default') { You shouldn't check for the existence of the "custom" directory; it's not required. >+ foreach my $lang (@langdirs) { >+ next if($lang =~ /^CVS$/i); >+ my $deft_path = File::Spec->catdir('template', $lang, 'custom'); >+ my $cust_path = File::Spec->catdir('template', $lang, 'custom'); Are these supposed to be identical? ;-) Gerv

> >+ if( ! -d 'template/'.trim($language).'/custom' > >+ && ! -d 'template/'.trim($language).'/default') { > > You shouldn't check for the existence of the "custom" directory; it's not > required. I do: if(either custom or default exists) then ok, adding a not before the () expands to: if( (not custom) AND (not default) ) then error > >+ my $deft_path = File::Spec->catdir('template', $lang, 'custom'); > >+ my $cust_path = File::Spec->catdir('template', $lang, 'custom'); > > Are these supposed to be identical? ;-) Of cause not, but I wanted to wait for some nits before updating that file. Interdiff: - my $deft_path = File::Spec->catdir('template', $lang, 'default'); + my $deft_path = File::Spec->catdir('template', $lang, 'custom');

did you swap the wrong one? I would think $cust_path is the one you would want to have 'custom' on the end of. :)

Comment on attachment 130218 [details] [diff] [review] v2.1: Initalize $deft_path by 'default' not 'custom' > did you swap the wrong one? I think I initially did, but then I corrected it (and missed the interdiff ;). Anyway, the patch is correct and can be reviewed.

Comment on attachment 130218 [details] [diff] [review] v2.1: Initalize $deft_path by 'default' not 'custom' r=gerv. Looks good to me. Gerv

a=myk. Please clear your review request of bbaetz before checking in if you don't need him to review anymore.

FIXED Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v <-- defparams.pl new revision: 1.118; previous revision: 1.117

What about, even as extra option, to automatically determine de languages parameter by listing the existing directories under the template directory?

This was the cause of bug 226217. Gerv