126955 - (bz-l10n) Bugzilla should support translated/localized templates

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Tobias Burnus]

Bugzilla should support a translated user interface.

The first and easierst step is to translate the templates which is almost enough
for the beginning since those contain most of the stings to be translated.
(There are two kinds of installations: Those which use only one language and
those which support bugreports in several languages)

Proposed new directory layout (inspired by /usr/share/man/):

templatex/default/ -- default language (en)
templatex/default/<languagecode>/ -- other languages (such as de, en-GB,...)
templatex/custom/ -- default language (en)
templatex/custom/<languagecode>/ -- other languages

The cgi's should then support the Accept-Language header to choose the template
(this is not the easiest task since the Accept-Language header can contain
things like fr-FR which should match fr-FR and fr ...)


I have a "default/de/" ready sitting here, if someone wants to check it in, drop
a mail. (This is actually independend of having the neede perl support since one
can us it to replace the English version in default.

And my have the time to dig into the perl part. checksetup.pl should probably
used to create a list of supported languages and the admin should probably have
the possibility to restrict the languages to a subset (this sounds a bit like
hacking defparms.pl). Additionally a link list where one can choose the desired
language would be neat (such as %commandmenu%).

Comment 1

[Marcus Fellinger]

I'm interested in the "/default/de/" template-set. I've set up a
bugzilla-installation under http://www.felliweb.de (click on Bugzilla). I intend
it to be used as a demonstration system for the german translation of bugzilla.

I also talked with KaiRo about translating bugzilla, and he seemed interested. 

If we are going to translate Bugzilla into other languages, the use of non-ASCII
characters in bug reports must not lead to invalid mail messages (Bug #110692)

Comment 2

[Dave Miller [:justdave]]

There are some major issues here related to directory structure within the
bugzilla hierarchy in order to facilitate this, which need to be resolved before
2.16 is released, whether or not any localized templates are actually included
with 2.16.  I will post a message to get some discussion of this going on
news:netscape.public.mozilla.webtools (mozilla-webtools mailing list), as it
needs a broader audience and participation than the people watching this bug.

Comment 3

[Dave Miller [:justdave]]

per my last comment, moving this to 2.16 so we don't forget to deal with the
directory structure before release.

Comment 4

[Arne Schirmacher]

checksetup.pl contains strings that should be localized ("This bug has been
marked as a duplicate..."), because they are written to the db. Can we use
templates here too?

Error and diagnostic messages should probably left as they are.

Comment 5

[Dave Miller [:justdave]]

bug 135707 is going to satisfy the 2.16 requirements for this.  Moving this bug
out to get it off the 2.16 list.

Comment 6

[Tobias Burnus]

Attachment: First version of a HTTP_ACCEPT_LANGUAGE patch

This is the first version of a patch which allows multiple languages based on
the HTTP_ACCEPT_LANGUAGE header as per discussion on
news://netscape.public.mozilla.webtools.

The supported languages can be set in editparms.cgi. If no suitable
HTTP_ACCEPT_LANGUAGE is found, the first language is used.
Note that I include the all languages from the list (editparms.cgi) since I
want to honour the case that not all templates have been translated.

Comment 7

[Tobias Burnus]

Attachment: Second version: Now searches custom, default instead of twice custom ;-)

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 86617 [details] [diff] [review]
Second version: Now searches custom, default instead of twice custom ;-)

Oops, sorry about that - wrong button.

>+sub TemplateIncludePath () {
>+  my $acceptheader = $ENV{HTTP_ACCEPT_LANGUAGE} || "";
>+  my $availlangs   = Param("languages");

Normally, we name the variable we copy a param into after that param.

>+  $acceptheader =~ s/;q=0-9//g;      # remove language priority and assume that they are ordered

This won't remove all q values, which can have a decimal point. You want
[0-9\.]+
or something like it.

>+  $acceptheader =~ s/[^A-Z_a-z,]//g; # taint
>+  $availlangs   =~ s/[^A-Z_a-z,]//g;

Are language and sub-language parts not ever hyphen-separated?

>+  # Try one: Match exactly: 'en_US' matches 'en_US', 'en' matches 'en' and 'en_US' etc.
>+  foreach(split /,/, $acceptheader) {
>+    if ( $availlangs =~ m/[^\w]($_\w*)/ ) {

If [^\w] means non-word char, use \W. And this will fail if it's first in the
list.
You want: 
my @acceptlangs = split(',', $acceptheader);
if grep($_, @acceptlangs) {
  $usedlang = $_;
  last;
}

and then
if grep("$__\w+", @acceptlangs) {

Basically, the aim of this function is to create a list of languages to try,
where the list is the intersection of ACCEPT_LANGUAGE and the Param, with the 
default appended to the end (it may appear twice; that's not a problem.)
You then create the template search path from this list using map() and
join(":", ...).

>+DefParam("languages",
>+         "A comma-separted list of <a href=\"http://lcweb.loc.gov/standards/iso639-2/englangn.html\">ISO 639 language codes</a> of the languages in which Bugzilla output may be displayed. The ACCEPT_LANGUAGE header is used to determin the actually used language, if no suitable ACCEPT_LANGUAGE is available, the first language is used.",

"A comma-separated list of <a
href=\"http://lcweb.loc.gov/standards/iso639-2/englangn.html\">ISO 639 
language codes</a> for the languages in which you wish Bugzilla output to be
displayed.  
Note that you must install the appropriate language packs before adding
languages
to this Param. The language used is the first language in this list which
is present in the ACCEPT_LANGUAGE header which the user's browser sends. If
there are
no matches, Bugzilla falls back on the first language in this list."

Gerv

Comment 9

[Tobias Burnus]

Attachment: Third version: Clean up a bit and make use of Gerv's suggestions

Gerv wrote: 
> Normally, we name the variable we copy a param into after that param.
fixed.

> Are language and sub-language parts not ever hyphen-separated?
I haven't found a RFC instancely, but Mozilla uses en-us whereas under UNIX
locale there is en_US. (I tend to mix them; thus I match both of them.)

> if grep($_, @acceptlangs) {
This will never work. As `man perlfunc` says: "`$_' is a reference into the
list value" and thus you cannot use foreach(@foo) {grep(/$_/,...

and then
if grep("$__\w+", @acceptlangs) {

> You then create the template search path from this list using map() and
> join(":", ...).
done.

>+DefParam("languages",
Used this DefParam explanation.

Comment 10

[Tobias Burnus]

Attachment: Fourth version: Fix 'use strict' error (add twice a 'my')

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 86927 [details] [diff] [review]
Fourth version: Fix 'use strict' error (add twice a 'my')

>+sub TemplateIncludePath () {

Nit: call it GetTemplateIncludePath().

>+  my $languages = Param("languages");
>+  $languages =~ s/[^A-Z_a-z,-]//g;
>+  my @languages = split (',', $languages);
>+
>+  my $accept_language = $ENV{HTTP_ACCEPT_LANGUAGE} || "";
>+  # remove language priority and assume that they are ordered
>+  $accept_language =~ s/;q=[0-9\.]+//g;
>+  # taint
>+  $accept_language =~ s/[^A-Z_a-z,-]//g;
>+  my @accept_language = split(',', $accept_language);

Rearrange as above for clarity, and rename @accept_language to 
@accept_languages.

>+  my $usedlang = "";
>+
>+  # Try one: Match exactly: 'en-us' matches 'en-us', 'en' matches 'en' and 'en-us' etc.
>+  foreach(@accept_language) {
>+    my $al = $_;
>+    if(my @found = grep(/$al/i, @languages)) {
>+      $usedlang = $found[0];
>+      last;
>+    }
>+  }
>+
>+  # Try two: en-us matches 'en'
>+    if( $usedlang eq '') {
>+    foreach(@accept_language) {
>+      my $al = $_;
>+      $al =~ s/[-_].*//;
>+      if(my @found = grep(/$al/i, @languages)) {
>+         $usedlang = $found[0];
>+         last;
>+      }
>+    }
>+  }
>+
>+  my $includepath = join(':', map("template/$_/custom:template/$_/default",@languages));
>+
>+  if($usedlang ne '') {
>+    $includepath = "template/$usedlang/custom:template/$usedlang/default:$includepath";
>+  }
>+  return $includepath;

I don't think this quite does the right thing. It creates an includepath 
consisting of a single language from accept_languages, plus the entire language 
list. What you want is an include path consisting of:
1) all the languages both in ACCEPT_LANGUAGES and the Param, in the order
   they appear in ACCEPT_LANGUAGES (except more specific ones first).
2) the default language from the Param (which should be guaranteed to have
every
   template.)

This makes it as likely as possible that the user will be given a template with
a 
language they understand.

This is how it should work. I really think the default language should be its
own
param - anything else is counter-intuitive. At the front of the list is wrong,
because it's the last place to look. At the back of the list is wrong, because
it implies they are searched in the order they are in the Param, which they
are not (they are searched in ACCEPT_LANGUAGE order.)

So, Param('languages') is an _unordered_ list.

Say you have Params as follows:
Param('defaultlanguage') = "en";
Param('languages') = "de-AT, fr, en";

What should the following accept-headers give as search paths?
ACCEPT1: "de";
This person should get "en". This is because by calling the localisation
"de-AT",
the admin is saying it is not suitable for other Germans speakers apart from
those in Austria. If it was suitable for all, it would be called "de". 
This is a key point.

ACCEPT2: "de-AT, en";
This person should get "de-AT", and then "en".

ACCEPT3: "fr, de-AT";
This person should get "fr" then "de-AT", then "en", because order in ACCEPT 
takes precedence over order in the Param.

So, how to implement this?
The algorithm is as follows (in pseudocode):

foreach my $accept_language (@accept_languages) {
  grep($accept_language, @param);
  if (match) {
    push(match, @include_languages);
  }

  $accept_language =~ s/([-_].*)//;
  if ($1) {
    grep($accept_language, @param);
    if (match) {
      push(match, @include_languages);
    }
  }
}

push(default, @include_languages);
join, map etc.

Gerv

Comment 12

[Tobias Burnus]

> What should the following accept-headers give as search paths?
> This person should get "en". This is because by calling the 
> localisation"de-AT", the admin is saying it is not suitable for other Germans 
> speakers apart from those in Austria. If it was suitable for all, it would be 
> called "de". This is a key point.
That way ok, but I strongly believe a HTTP_ACCEPT_LANGUAGE header with de-at
should also match to languages 'de'. Usually this doesn't mean that they are
opposed to 'de' but that they favour 'de-at'. (Yes they should then use
'de-at,de' but they ususally don't to.) More over as a simple user I wont be
able to specify all that languages that there are available:
en,en-us,en-uk,en-uk-southwestlondon, en-uk-myowndialect etc.

Comment 13

[Tobias Burnus]

Attachment: Fifth version: Complete rework

I now actually use the ;q=... values of the Accept-Language header (RFC 2616, I
finally found the standard). I reworked additionally the logic according to
both the standard and to Gerv (therefore I also introduced
Param('defaultlanguage').
Additionally the code is -- in my humble opinion -- much cleaner, shorter and
more beautiful.

Comment 14

[Tobias Burnus]

Gerv wrote and I included it also in the latest patch:
> The language used is the first language in this list which is present in the
> ACCEPT_LANGUAGE header which the user's browser sends.
This is actually not true (and never was). It is rather the otherway round: The
first language (with the highst q-value or which comes first (same q-value))
which matches the Param(languages) is used (and all other matching languages are
also added to the path).
Can someone formulate this in nice plain English?

I found another nit in my code:
>+  foreach(split /,/, $accept_language) {
>+    my ($lang, $qvalue) = split /;q=/;
 next if not defined $lang;
is missing (to prevent a warning if a '*' is present, which is valid according
RFC 2616).

Comment 15

[Thomas Ganter]

I, unable to do a correct query did not find these patches and wrote my own,
available with bug 152556. 

However, bug 152556 takes the question one step further: Loads of content are
generated directly from the perl source and are thus not influenced by the
templates. 

One example are the titles for the bug list. Another one are the bug stati
('ASSIGNED' never shows up in any template). 

Using templates for each of these sureley would be possible, but IMHO
unpractical. We should be comming up with some translation code that gets the
english word or short phrase thrown upon and then comes up with a translation. 

Translations could then be read from flat files, one per language. 

Comment 16

[Dave Miller [:justdave]]

*** Bug 152556 has been marked as a duplicate of this bug. ***

Comment 17

[Gervase Markham [:gerv]]

Thomas: we have plans for how to localise the rest of the Bugzilla interface,
probably using gettext(). If you have ideas, post them to the newsgroups.

Gerv

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 86948 [details] [diff] [review]
Fifth version: Complete rework

>+  my @accept_language = sortAcceptLanguage($ENV{HTTP_ACCEPT_LANGUAGE} || "");

AFAIAA, Bugzilla style is to use quotes on hash keys. You do
this in a couple of places.

>+DefParam("languages",
>+         "A comma-separated list of RFC 1766 language tags of languages in which you wish Bugzilla output to be displayed. Note that you must install the appropriate language packs before adding languages to this Param. The language used is the first language in this list which is present in the Accept-Language header which the user's browser sends.",

"A comma-separated list of RFC 1766 language tags. These identify the languages
in 
which you wish Bugzilla output to be displayed. Note that you must install 
the appropriate language pack before adding a language to this Param. The 
language used is the one in this list with the highest q-value in the user's 
Accept-Language header."

>+DefParam("defaultlanguage",
>+         "The language Bugzilla falls back on if no matching Accept-Language header can be found.",

"The UI language Bugzilla falls back on if no suitable language is found in the
user's Accept-Language header."

Other than those nits, this stuff is perfect :-)

Gerv

Comment 19

[Tobias Burnus]

Attachment: Sixth version: Updated param descriptions, use quotes in hash references

Comment 20

[Tobias Burnus]

Attachment: Seventh version: Fix '*' problem mentioned in comment 14

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 88320 [details] [diff] [review]
Seventh version: Fix '*' problem mentioned in comment 14

>+# Make an ordered list out of a HTTP Accept-Language header see RFC 2616, 14.4
>+# We ignore '*' and <language-range>;q=0
>+# For languages with the same priority q the order remains unchanged.
>+sub sortAcceptLanguage ($) {
>+  sub sortQvalue { $b->{'qvalue'} <=> $a->{'qvalue'} }

Bugzilla standard is a 4-space indent, and we don't use function prototypes.

>+  # clean up string.
>+  $accept_language =~ s/[^A-Za-z;q=0-9\.\-,]//g;

Isn't using "tr" more efficient here? I don't know...

>+    push(@qlanguages,{ 'qvalue' => $qvalue, 'language' => $lang});

Nit: push(@qlanguages, {'qvalue' => $qvalue, 'language' => $lang}); 

>+  push(@usedlanguages,Param('defaultlanguage'));

Nit: push(@usedlanguages, Param('defaultlanguage'));

Gerv

Comment 22

[Tobias Burnus]

Attachment: Eighth version: Whitespace fixes, remove ($) prototyping

Gerv wrote:
>(From update of attachment 88320 [details] [diff] [review])
> >+sub sortAcceptLanguage ($) {
> >+  sub sortQvalue { $b->{'qvalue'} <=> $a->{'qvalue'} }
> Bugzilla standard is a 4-space indent, and we don't use function prototypes.
Fixed spacing and removed '($)'.

> >+  $accept_language =~ s/[^A-Za-z;q=0-9\.\-,]//g;
> Isn't using "tr" more efficient here? I don't know...
I failed to get  tr  working (delete everything except ..., for me only delete
nothing except ... worked), but I'm not a PERL specialist.

>Nit: push(@qlanguages, {'qvalue' => $qvalue, 'language' => $lang}); 
>Nit: push(@usedlanguages, Param('defaultlanguage'));
Fixed those and a few more.

Comment 23

[Gervase Markham [:gerv]]

Comment on attachment 88445 [details] [diff] [review]
Eighth version: Whitespace fixes, remove ($) prototyping

r=gerv.

Gerv

Comment 24

[Gervase Markham [:gerv]]

Fixed.

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.185; previous revision: 1.184
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.82; previous revision: 1.81
done

Gerv

Comment 25

[Joel Peshkin]

This seems to have caused a regression.

If a new installation runs checksetup with a fresh-checkout where the data
directory has not even been created, checksetup requires globals, globals now
checks a Param, (languages), which causes it to realize that Params aren't loaded.

When defparams runs, it chokes if the data directory doesn't yet exist.

Comment 26

[Bradley Baetz (:bbaetz)]

'joy'.

We need to up the requirement to TT 2.08, and then pass a sub ref in as the
INCLUDE_PATH param, which will remove those issues.

You also totally ignored the directive in globals.pl to update checksetup.pl at
t/004template.t with the new params....

Those two need to end up searching _all_ existing template paths, though, so
you'll need to change the File::Find calls, too.

Comment 27

[Dave Miller [:justdave]]

I'm going to back out the existing patch at 9pm PDT (-0700) tonight unless we
have a corrective patch to fix it by then, in the interest of keeping the cvs
tip at least usable.  You have T minus 2 hours, 3 minutes, and counting.

Comment 28

[Dave Miller [:justdave]]

Backed out.

tcsh> cvs update -j1.82 -j1.81 defparams.pl
RCS file: /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v
retrieving revision 1.82
Merging differences between 1.82 and 1.81 into defparams.pl
tcsh> cvs update -j1.185 -j1.184 globals.pl
RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
retrieving revision 1.185
retrieving revision 1.184
Merging differences between 1.185 and 1.184 into globals.pl
tcsh> cvs commit
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.85; previous revision: 1.84
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.191; previous revision: 1.190
done

Comment 29

[Gervase Markham [:gerv]]

I'll revisit this next week. bbaetz: what are "those two"?

Gerv

Comment 30

[Gervase Markham [:gerv]]

justdave, myk, bbaetz:
bbaetz says the easiest solution here is to up the TT required version to 2.08.
Is that feasible?

Gerv

Comment 31

[Bradley Baetz (:bbaetz)]

Yeah, that should work. Pass a sub ref in within an array ref. see perldoc
Template::Provider.

Note that this subref will be called for every template/INCLUDE/PROCESS, so it
should be fast. What you should do is compute an array of available template
params (and cache it), then for each entry in the accept lang stuff, check for
an exact match, or a prefix match (but see below). You can also (as of 2.08)
return an arrayref of entries, rather than a string.

However, I'm wondering how this will work with non-CGI templates.

Also, the patch is wrong - en-gb does not match against en, although en does
match against en-gb. See the last paragraph of rfc2616 section 14 (theres a
mozilla bug somewhere to improve the ui) As well, order matters - if en is
before en-GB, then the generic en templates should be used even if en-GB exists.

Comment 32

[Tobias Burnus]

> Also, the patch is wrong - en-gb does not match against en, although en does
> match against en-gb.

# @accept_language = HTTP_ACCEPT_LANGUAGE; @languages = Param('languages')
+    foreach my $lang (@accept_language) {
+        # match exactly (case insensitive)
+        if(my @found = grep /^$lang$/i, @languages) {
+            push (@usedlanguages, $found[0]);
+        }

This should match 'en-gb' as 'en-gb', shouldn't it? And 'en' should only match
'en' and not 'en-gb'. Or do I miss something obvious?

> As well, order matters - if en is before en-GB, then the generic en templates 
> should be used even if en-GB exists.
I believe this is the case (with the exception that the q-values take
precedence) since I check for every HTTP_LANGUAGE_HEADER whether this language
is present in the Param('languages') and push it if it is the case. Then I
remove the '-.*' and push it also - if needed.

The only thing that I am aware of which doesn't match RFC 2616 is that
'en-gb-cockney' doesn't look also for 'en-gb' but only for 'en'.

For the order, there may be a problem buried: The sort algorithm may change the
order for items with the same (or no) q value depending on the algorithm used by
PERL. (This shouldn't happen with Mozilla since it passed the q-values and with
@language = 'de','en' it works also well with Netscape 4.x -- even it I use
@HTTP_ACCEPT_LANGUAGE = 'de-de', 'en'.)

Comment 33

[Bradley Baetz (:bbaetz)]

en in ACCEPT_LANGUAGES is a valid match for an en-GB template, but not the other
way arround.

Also, for the template lang foo-bar-baz, you need to test if foo-bar and foo
match those (Which they do, per the RFC)

Comment 34

[Gervase Markham [:gerv]]

> Also, for the template lang foo-bar-baz, you need to test if foo-bar and foo
> match those (Which they do, per the RFC)

Given that this will complicate the code, and the fact that the vast majority of
codes are two-part, I think that this enhancement can be made when the first
localisation which actually uses a three-or-more-part language code is proposed :-)

Gerv

Comment 35

[Tobias Burnus]

Attachment: Nineth version: Rediff due to changes, check -e data/params

I "rediffed" the eight patch due to the changes in defparams.pl.
This version checks wether data/params exists and if not returns a default
path.
The generated path is now also used in ./checksetup.pl.

This does't include changes to t/004template.t (yet). And I don't use the TT
2.08 feature to pass a sub to INCLUDE_PATH (which would have the advantage that
there is not yet another location where data/params).

The main reason for this patch is to provide a working solution (TM) for users
of e.g. http://bugzilla-de.sf.net/.

Comment 36

[Bradley Baetz (:bbaetz)]

Comment on attachment 101093 [details] [diff] [review]
Nineth version: Rediff due to changes, check -e data/params

Yeah, but you can't do that. The problem is that you need to precompile _all_
the templates - ie checksetup and the tests need to traverse each of
template/*/{custom,default}, otherwise you get permission problems when apache
writes stuff, and so on.

Comment 37

[Tobias Burnus]

Attachment: Tenth version: Needs TT 2.08(a), uses File::Find (checksetup.pl) and uses a subref

I really failed to get this work with TT 2.08, it works with TT 2.08a though.
Maybe this is only due to my broken setting or the broken program code
(Debugging is really broken in TT 2.08 thus I needed TT 2.08a to fix my code).
Left to do as an exercise: The t/* stuff.

Comment 38

[Tobias Burnus]

"Internal" Testing result: It works with both TT 2.08 and TT 2.08a, i.e. I've
only needed TT 2.08a for debuging purpose (debugging is broken in TT 2.08).

I need to fix this though (TT 2.08/2.08a): doeditparams.cgi shows the following,
if I edit defaultlanguage or languages:
--------------------------------------------------------
Saving new parameters: Changed defaultlanguage
We don't have shadow databases turned on; no syncing performed.
OK, done.   Edit the params some more.  Go back to the query page. 

Bugzilla has suffered an internal error. Please save this page and send it to
ZEDV Bugzilla Maintainer  with details of what you were doing at the time this
message appeared.
URL: http://bugzilla.xxxx.yyyy/doeditparams.cgi
Template->process() failed twice.
First error: Insecure dependency in rename while running with -T switch at
/usr/lib/perl5/Template/Document.pm line 286.
Second error: Insecure dependency in rename while running with -T switch at
/usr/lib/perl5/Template/Document.pm line 286.
----------------------------
Since everything is saved, it works, but it doesn't look very nice. Any ideas on
how to fix this are welcome.

Comment 39

[Tobias Burnus]

Attachment: Eleventh version: Fix caching of the template include path

The "Insecure dependency in rename while running with -T switch at
/usr/lib/perl5/Template/Document.pm line 286." was due to the fact that I
didn't cache the template include path during debugging (see the '#')
+    return #$::vars->{'template-includepath'} =
+	 [split /:/, join(':', map("template/$_/custom:template/$_/default",
@usedlanguages))];

Fixed now. I'd like to see how this is handled when used as mod_perl...
Please review.

Comment 40

[Gervase Markham [:gerv]]

Comment on attachment 102583 [details] [diff] [review]
Eleventh version: Fix caching of the template include path

This looks excellent. We can check it in as soon as we've coordinated the
upgrade to TT 2.08 with the developers.

I have one other query - would it be worth implementing an optimisation for
when Param{'languages'} is a single value, which will be the common case
(usually "en")?

+    if ($::vars->{'template-includepath'}) { 
+	 return $::vars->{'template-includepath'};
+    }

my $languages = Param{'languages'};
if (length($languages) == 2) {
    return "template/$languages/custom:template/$languages/default";
}

Gerv

Comment 41

[Bradley Baetz (:bbaetz)]

Comment on attachment 102583 [details] [diff] [review]
Eleventh version: Fix caching of the template include path

>+sub getTemplateIncludePath () {
>+    # Return cached value if available
>+    if ($::vars->{'template-includepath'}) { 
>+        return $::vars->{'template-includepath'};
>+    }

This doesn't belong in $::vars - use some other global instead

>+    my @languages = ();
>+    {
>+        use File::Find;
>+        sub languageDirectories {
>+           my ($dev,$ino,$mode,$nlink,$uid,$gid);
>+           $File::Find::name =~ m/^[^\/]*\/([a-z-]*)$/i && $_ ne 'CVS'
>+           && (($dev,$ino,$mode,$nlink,$uid,$gid) = lstat($_)) && -d _
>+           && push(@languages, $_);
>+        }
>+        find({wanted => \&languageDirectories, bydepth => 1}, 'template');
>+    }

Why do you need lstat? what is that checking for?

>     my $redir = ($^O =~ /MSWin32/i) ? "NUL" : "/dev/null";
>     my $template = Template->new(
>       {
>@@ -928,7 +940,7 @@
>         OUTPUT => $redir,
> 
>         # Colon-separated list of directories containing templates.
>-        INCLUDE_PATH => "template/en/custom:template/en/default",
>+        INCLUDE_PATH => join(':', map("template/$_/custom:template/$_/default",@languages)) ,
> 

This won't work, because this will only then find stuff form the first
language, not all teh languages. You need to do this one language at a time,
for all supported languages, with the appropriate INCLUDE_PATH.

Comment 42

[Tobias Burnus]

Attachment: Twelfth version: fix checksetup compilation, other review fixed

Gerv wrote:
> I have one other query - would it be worth implementing an optimisation for
> when Param{'languages'} is a single value, which will be the common case
> (usually "en")?
included.

Bradley Baetz wrote:
> This doesn't belong in $::vars - use some other global instead
fixed

Additionally I changed the compilation of the templates in checksetup. This is
based on the attachment 102666 [details] [diff] [review] of bug 168191 and basically adds a foreach and
moves the code a bit around.

I still haven't updated t/004template.t and t/Support/Templates.pm yet

Comment 43

[Bradley Baetz (:bbaetz)]

Comment on attachment 102670 [details] [diff] [review]
Twelfth version: fix checksetup compilation, other review fixed

This looks fine. You can drop teh definition of $redir in checksetup (since its
no longer used).

Update the other files, and I'll r= this

Comment 44

[Bradley Baetz (:bbaetz)]

ping?

Comment 45

[Dave Miller [:justdave]]

Yeah, we need to get this in...  we already have two localizations other than
English available, and we need to stop forcing them to say "change this line in
globals.pl to use these".

Comment 46

[Tobias Burnus]

Attachment: Thirteenth version: Rediff + modification to the t/* tests

- Rediff due to the checkin of bug 168191
- Finally updated t/*
(Plus fix warning in t/007util.t)

Sorry that it took that long, but I had to learn for an exam.
(This breakes the current patch to bug 173622, but I don't care and a rediff is
easily done ;-)

Comment 47

[Gervase Markham [:gerv]]

Comment on attachment 107031 [details] [diff] [review]
Thirteenth version: Rediff + modification to the t/* tests

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.204
>diff -u -r1.204 checksetup.pl
>--- checksetup.pl	19 Nov 2002 07:19:30 -0000	1.204
>+++ checksetup.pl	21 Nov 2002 17:02:00 -0000
>@@ -1,4 +1,4 @@
>-#!/usr/bonsaitools/bin/perl -w
>+#!/usr/bin/perl -w
> # -*- Mode: perl; indent-tabs-mode: nil -*-
> #
> # The contents of this file are subject to the Mozilla Public
>@@ -25,6 +25,7 @@
> #                 Zach Lipton  <[e-mail]>
> #                 Jacob Steenhagen <[e-mail]>
> #                 Bradley Baetz <[e-mail]>
>+#                 Tobias Burnus <[e-mail]>
> #
> #
> # Direct any questions on this source code to
>@@ -944,33 +945,26 @@
>         finddepth(\&remove, 'data/template');
>     }
> 
>+    # Search for template directories
>+    # We include the default and custom directories separately to make
>+    # sure we compile all templates
>+    my @templatepaths = ();
>+    {
>+        use File::Find;
>+        sub languageDirectories {
>+           my ($dev,$ino,$mode,$nlink,$uid,$gid);
>+           if(   $File::Find::name =~ m/^[^\/]*\/([a-z-]*)$/i

Nit: standard formatting is 
if ($File::...

>+        foreach $::templatepath(@templatepaths) {

Again, a nit: Space after $::templatepath.

>+           $::provider = Template::Provider->new(
>+             {

And line the opening bracket up with the $ sign.

>     # Colon-separated list of directories containing templates.
>-    INCLUDE_PATH => "template/en/custom:template/en/default" ,
>+    INCLUDE_PATH => [\&getTemplateIncludePath],

Remind me: does this require a bump in the required version of TT?

> +    my $languages = Param('languages');
> +    if (length($languages) == 2) {
> +        return $::template_include_path =
> +               ["template/$languages/custom", "template/$languages/default"];
> +    }

Would it not be better to check for the existence of a comma, instead of the
length? (A comma would indicate multiple values). Also, trim() the languages
Param when you read it, to avoid nasty problems with this shortcut.

Gerv

Comment 48

[Bradley Baetz (:bbaetz)]

Comment on attachment 107031 [details] [diff] [review]
Thirteenth version: Rediff + modification to the t/* tests

Don't change teh #! line

+    
+    eval("use Template");
+    my $redir = ($^O =~ /MSWin32/i) ? "NUL" : "/dev/null";

You don't need $redir

+	 # Per RFC 1766 and RFC 2616 any language tag matches also its 
+	 # primary tag. That is en-gb matches also en but not en-uk
+	 $lang =~ s/(-.*)//;
+	 if($1 ne '') {

Just test $1 - if its not there, $1 will be undef, and there will be warnings
comparing it to ''.

+	 [split /:/, join(':', map("template/$_/custom:template/$_/default",
+				   @usedlanguages))];

Why are you splitting and then joining?

Also, this may be better to do after my patch - instead of caching the include
path in a $:: global var, you can calculate it whn the user joins, in hte
Bugzilla object. IF this gets in before that, though, then I guess I'll do it.

You also do need to bump the requried TT version.

Also, zach or someoe else familiar with the tests shold look at those changes.

Comment 49

[Tobias Burnus]

Attachment: Fourteenth version: Fix review nits.

> Remind me: does this require a bump in the required version of TT?
It does. Somehow it got lost (it was present in tenth version).

> Would it not be better to check for the existence of a comma, instead of the
> length?
Did so now.

> You don't need $redir
Now I finally delete it (should have done so after comment 43).

> Don't change teh #! line
Good catch! This is the disadvantage of using a real world installation for
testing.

> Why are you splitting and then joining?
Because I wasn't aware of the fact that map accepts a list as first parameter
(and didn't try whether it works - it does).

This additionally fixes bug 162224 (t/006spellcheck.....Use of uninitialized
value). [006spellcheck uses tons of tabs, maybe t/005no_tabs.t should also
check the t/* files ...]

Comment 50

[Bradley Baetz (:bbaetz)]

Comment on attachment 107132 [details] [diff] [review]
Fourteenth version: Fix review nits.

>Index: checksetup.pl

>+    my @templatepaths = ();
>+    {
>+        use File::Find;
>+        sub languageDirectories {
>+           my ($dev,$ino,$mode,$nlink,$uid,$gid);

What are those for?

>+           if($File::Find::name =~ m/^[^\/]*\/([a-z-]*)$/i
>+              && $_ ne 'CVS' && -d $_) {

You don't need the bracketing. Also, will this work with windows, where it uses
\ ?

Rather than using File::Find, aren't you better off doing readdir for
'template', and then using File::Spec to join the paths for custom/default?

>         return if ($name =~ /\/CVS\//);
>         return if ($name !~ /\.tmpl$/);
>-        $name =~ s!template/en/default/!!; # trim the bit we don't pass to TT
>+        $name =~ s!$::templatepath/!!; # trim the bit we don't pass to TT

s/\Q$::templatepath\E// in case the lang code contains a ., or something.

> 
>         chdir($::baseDir);
> 
>         # Do this to avoid actually processing the templates
>-        my ($data, $err) = $provider->fetch($name);
>+        my ($data, $err) = $::provider->fetch($name);

If it makes it easier, you can also use no_chdir for File::Find now that
require perl 5.6.

>+           # Disable warnings which come from running the compiled templates
>+           # This way is OK, because they're all runtime warnings.
>+           # The reason we get these warnings here is that none of the required
>+           # vars will be present.
>+           local ($^W) = 0;

Actually, you can drop this now that we no longer run the templates, just
compile them

>--- t/007util.t	10 Nov 2002 23:31:15 -0000	1.1
>+++ t/007util.t	22 Nov 2002 10:20:09 -0000
>@@ -63,7 +63,7 @@
> is(lsearch(\@list,'kiwi'),-1,'lsearch 3 (missing item)');
> 
> #max() and min():
>-my @list = (7,27,636,2);
>+@list = (7,27,636,2);
> is(max(@list),636,'max()');
> is(min(@list),2,'min()');

Why this change?

Comment 51

[Tobias Burnus]

Attachment: Fiveteenth version: Further review fixes.

> >+	       my ($dev,$ino,$mode,$nlink,$uid,$gid);
> What are those for?
Leftovers from pfind (removed by rewrite of this section).

> Rather than using File::Find, aren't you better off doing readdir for
> 'template', and then using File::Spec to join the paths for custom/default?
did so now.

> s/\Q$::templatepath\E// in case the lang code contains a ., or something.
Done.

> If it makes it easier, you can also use no_chdir for File::Find now that
> require perl 5.6.
Did so.

> Actually, you can drop this now that we no longer run the templates, just
> compile them
Removed those bits.

> >-my @list = (7,27,636,2);
> >+@list = (7,27,636,2);
> Why this change?
Well it is basically:
  my @list = ('apple','pear','plum','<"\\%');
 -my @list = (7,27,636,2);
 +@list = (7,27,636,2);
which fixes a warning.

Comment 52

[Bradley Baetz (:bbaetz)]

Comment on attachment 107344 [details] [diff] [review]
Fiveteenth version: Further review fixes.

This is fine, with a few minor changes.

I do want zach (or someone) to review the tests stuff, though

>Index: checksetup.pl

>+        foreach my $dir (@files) {
>+            next if($dir =~ /^CVS$/i);
>+            my $path = File::Spec->catfile('template', $dir, 'custom');

This should be ->catdir, ratehr than catpath. Note that that way you get a
trailing /, which may affect some of the below stuff

>+            push(@templatepaths, $path) if(-d $path);
>+            $path = File::Spec->catfile('template', $dir, 'default');

ditto

>+            push(@templatepaths, $path) if(-d $path);
>+        }


>-        $name =~ s!template/en/default/!!; # trim the bit we don't pass to TT
>-
>-        chdir($::baseDir);
>+        $name =~ s/\Q$::templatepath\/\E//; # trim the bit we don't pass to TT

The trailing / should be outside the \Q\E. |perldoc perlre| refers to 'gory
details' with backslashes inside \Q\E. Besides, \ isn't technically portable,
although its accepted on all the OSs we support, I guess.

>@@ -60,6 +61,7 @@
>     $zz = $main::template;
>     $zz = $main::userid;
>     $zz = $main::vars;
>+    $zz = $main::template_include_path

This is ugly, but I'm rewriting that over in the other bug, so don't worry
about it.

>+    # clean up string.
>+    $accept_language =~ s/[^A-Za-z;q=0-9\.\-,]//g;
>+    my @qlanguages;
>+    my @languages;
>+    foreach(split /,/, $accept_language) {
>+        my ($lang, $qvalue) = split /;q=/;

Use a regexp rather than split, just in case someone does something silly with
two qvalues. ie en_US;q=0;q=1

if (m/([A-Za-z\.\-]+)(?:;q=(\d+(?:\.\d+)))?/) {
  $lang = $1;
  $qval = $2

I think - do test, esp teh decimal point stuff

(That regexp is meant to accept one or more of any letter, dot, or -, then
optionally ;q=, some digits, and optionally .<some more digits>)

That does the 'cleanup' there, too, and also more accurately validates the
data.

I've skiped the tests stuf; I'm not really familar with it.

Comment 53

[Bradley Baetz (:bbaetz)]

Actually, forget that regexp for qvalues - I changed my mind, and its not worth
it. The rest still stands, though

However, this is also wrong:

+        # Per RFC 1766 and RFC 2616 any language tag matches also its 
+        # primary tag. That is en-gb matches also en but not en-uk
+        $lang =~ s/(-.*)//;
+        if($1) {
+            if(my @found = grep /^$lang$/i, @languages) {
+                push (@usedlanguages, $found[0]);
+            }
+        }

a) You need to test the result of the s//, not $1 - if it doesn't match, $1 will
be left at whatever it was last time it was used.

b) Also, as I mentioned earlier (see comment #33), this is the wrong way
arround. A _client_ (ie $lang) of en matches a _server_ value of en-US, not the
other way arround.

Comment 54

[Tobias Burnus]

Attachment: Sixteenth version: Fix the last nits of bbaetz.

> I do want zach (or someone) to review the tests stuff, though
will do an request.

> This should be ->catdir, ratehr than catpath
Fixed this with all implications.

> Use a regexp rather than split, just in case someone does something
> silly with two qvalues. ie en_US;q=0;q=1
> if (m/([A-Za-z\.\-]+)(?:;q=(\d+(?:\.\d+)))?/) {

> (That regexp is meant to accept one or more of any letter, dot, or -, then
well the language may not contain a dot, and the number starts with one 0 or a
1 followed (optionally) by a dot, so I have now:
  if (m/([A-Za-z\-]+)(?:;q=(\d(?:\.\d+)))?/) {
See http://www.ietf.org/rfc/rfc2616.txt, Sec. 14.4

Comment 55

[Tobias Burnus]

Comment on attachment 107354 [details] [diff] [review]
Sixteenth version: Fix the last nits of bbaetz.

(request review)
Zach, can you especially look at the changes of the tests (t/004template.t
etc.)

Comment 56

[Bradley Baetz (:bbaetz)]

Comment on attachment 107354 [details] [diff] [review]
Sixteenth version: Fix the last nits of bbaetz.

You don't appear to have fixed teh language thing I mentioned in my followup
comment

Comment 57

[Tobias Burnus]

> You don't appear to have fixed teh language thing I mentioned in my followup
> comment
Since I didn't see it. (I attached without reloading the page first.)

> a) You need to test the result of the s//, not $1 - if it doesn't match, $1
> will be left at whatever it was last time it was used.
Going to fix this later. First I need to understand the following problem:

> b) Also, as I mentioned earlier (see comment #33), this is the wrong way
> arround. A _client_ (ie $lang) of en matches a _server_ value of en-US, not 
> the other way arround

I still believe this is correct:

ACCEPT        MATCHES
(http header) (available languages: e.g. en, en-gb)
en            en     Exact match
en-gb         en-gb  Exact match
(en-us)      (no exact match)
en-us => en   en     Main language part matching

And I believe this happens. It seems you want to do it the otherway round:
Accept=en  should match available=('en-us')?

See http://www.ietf.org/rfc/rfc2616.txt, section 14.4: "A language-range matches
a language-tag if it exactly equals the tag, or if it exactly equals a prefix of
the tag such that the first tag character following the prefix is "-"."

Comment 58

[Jacob Steenhagen]

Comment on attachment 107354 [details] [diff] [review]
Sixteenth version: Fix the last nits of bbaetz.

As long as you're gonna be uploading a new version anyway, here's my comments
regarding the changes to the tests...
>Index: t/004template.t
>===================================================================
>@@ -54,68 +55,84 @@
>     }
> }
> 
>-my $include_path = $Support::Templates::include_path;
>+my @languages     = @Support::Templates::languages;
>+my @include_paths = @Support::Templates::include_paths;
>+my %include_path  = %Support::Templates::include_path;

Instead of doing all this, you can export these variables from
Support::Templates
use base qw(Exporter);
@Support::Files::EXPORT = qw(@languages @include_paths %include_path);

>+sub existOnce {
>+  foreach(@_) {
>+    return $_  if -e $_;
>+  }
>+  return 0;
>+}

I have a tendeny to not like using $_ and @_ quite so much.  At the very least
I'd do:
foreach my $path (@_) {

>+        if (my $path = existOnce(@path)) {
>+           ok(1, "$path exists");
>+        } else {

Have you tested that? I'm pretty sure that the assignment will return true even
if it's assigning the value of '0' to $path (it seems like I got bit by that
before).
Also, that else should be on the next line (ie, not nested).

>+            ok(0, "neither ".join(' nor ',@path)." do exist --ERROR");

This error doesn't seem to be grammatically correct. Maybe something like:
ok(0, "$file cannot be located --ERROR");
print $fh "Looked in:\n  " . join("\n  ", @path);

>+    for my $I (0 .. $#{$Support::Templates::actual_files{$include_path}}) {
>+        my $file = $Support::Templates::actual_files{$include_path}[$I];
>+        my $path = File::Spec->catfile($include_path, $file);
>+        if (-e $path) {
>+            my ($data, $err) = $provider->fetch($file);
>+
>+            if (!$err) {
>+                ok(1, "$file syntax ok");
>+            }
>+            else {
>+                ok(0, "$file has bad syntax --ERROR");
>+                print $fh $data . "\n";
>+            }

I don't see $I being used anywhere but in the assignment of $file.  Wouldn't:
foreach my $file (@{$Support::Templates::actual_files{$include_path}}) {

make more sense?  Also, if you add @actual_files to what's exported from
Support::Templates then you can even leave the package name out (which is the
only reason to export the other variables in the first place).

>+    for my $I (0 .. $#{$Support::Templates::actual_files{$include_path}}) {
>+        my $file = $Support::Templates::actual_files{$include_path}[$I];
>+        my $path = File::Spec->catfile($include_path, $file);
>+        open(TMPL, $path);
>+        my $firstline = <TMPL>;
>+        if ($firstline =~ /\d+\.\d+\@[\w\.-]+/) {
>+            ok(1,"$file has a version string");
>+        } else {
>+            ok(0,"$file does not have a version string --ERROR");
>+        }
>+        close(TMPL);

Ditto.

>Index: t/Support/Templates.pm
>===================================================================
>-use vars qw($include_path @referenced_files @actual_files);
>+use vars qw(@languages @include_paths %include_path @referenced_files
>+            %actual_files $num_actual_files);

I'm pretty sure that if you export as suggested above you don't have to put
them in the |use vars| pragma. (Souldn't we be using 'our' now that we require
5.6?)

>+# Local subroutine used with File::Find
>+sub find_include_path {
>+    my ($dev,$ino,$mode,$nlink,$uid,$gid);
>+    if(   $File::Find::name =~ m/^[^\/]*\/([a-z-]*)$/i
>+       && $_ ne 'CVS' && -d $_) {
>+        push(@languages, $_) if(-d "$_/custom" or -d "$_/default");
>+        my @dir = ();
>+        push(@dir, "template/$_/custom")  if(-d "$_/custom");
>+        push(@dir, "template/$_/default") if(-d "$_/default");
>+        push(@include_paths, @dir);
>+        $include_path{$_} = join(":",@dir);
>+    }
>+}
>+

Shouldn't these be using File::Spec->catfile?

>+my @files;
> ...
>-        push(@actual_files, $filename);
>+        push(@files, $filename);
> ...
>+# Scan the given template include path for templates
>+sub find_actual_files {
>+  my $include_path = $_[0];
>+  @files = ();
>+  find(\&find_templates, $include_path);
>+  return @files;
>+}

I realize that I'm only looking at this in patch form, but it doesn't seem to
make much sense that you declare @files, you fill @files, then in the sub you
empty @files and apparently never use the other stuff you collected.

>+  $num_actual_files += $#{$actual_files{$include_path}} + 1;

That should really be:
$num_actual_file += scalar(@{$actual_files{$include_path}})

scalar(@array) returns the number of elements in the array whereas $#array
returns the index of the highest element.

scalar(@array) == $#array + 1  # <--- Is almost always true

and in all honesty, the scalar() part is assumed if you use the @array in
scalar conext, but I generally don't like assumptions :)

>-    open (FILE, $file);
>+    open (FILE, $file); 

I assume this is some kind of a whitespace only change?

>+

and this, too?

I think that's all I have...

Comment 59

[Tobias Burnus]

Attachment: Eighteenth version[w/o t/*]: rediff, fix language matching now that I understand RFC1766/2616

Finally rediffed the patch, now that bug 173622 [Move template handling into a
module] is fixed.
And I finally understood RFC1766/2616 (in the W3C commented version):
Someone who wants English ('en') doesn't care whether (s)he get British,
American, Australian,... English. But if one wants to get 'en-gb', offering
'en-us' is a no-no. - Thus this is fixed as well.

t/* update comes soon.

Comment 60

[Tobias Burnus]

Attachment: [real!] Eighteenth version: Fix bug + include updated t/*

> > This should be ->catdir, rather than catpath
> Fixed this with all implications.
Hmm, somehow it didn't survive. Now uses (finally) "catdir"!

Regarding Jake's t/* directory comments:
> Instead of doing all this, you can export these variables from
> Support::Templates
Did so now, though it makes some variables appear a bit out of nowhere this
way.

> >+	    if (my $path = existOnce(@path)) {
> >+	       ok(1, "$path exists");
> >+	    } else {
> Have you tested that?
I think I had, I tested it again and it actually seems to work.

> >+		ok(0, "neither ".join(' nor ',@path)." do exist --ERROR");
Changed to suggested wording.

> I don't see $I being used anywhere but in the assignment of $file.  Wouldn't:

> foreach my $file (@{$Support::Templates::actual_files{$include_path}}) {
I now used the suggested construction.

> Shouldn't these be using File::Spec->catfile?
Changed this section completely and brought it line with checksetup.pl
(which uses catdir (sic!))

Comment 61

[Gervase Markham [:gerv]]

(I'll re-review this once bbaetz or jake is happy with it.)

Gerv

Comment 62

[Tobias Burnus]

PING
Anyone who wants to (re)review?
(t/006spellcheck.t will fail due to be better fix from bug 166481; ignore that.)

Comment 63

[Tobias Burnus]

Comment on attachment 109949 [details] [diff] [review]
[real!] Eighteenth version: Fix bug + include updated t/*

Gerv:
> (I'll re-review this once bbaetz or jake is happy with it.)
Ok, but I added you nevertheless as addl. review

Comment 64

[Jacob Steenhagen]

I'd like to find the time to re-review this, but college is kicking my butt
right now.  In fact, I need to be starting my English paper that's due tomorrow
instead of typing this comment...

Comment 65

[Gervase Markham [:gerv]]

Comment on attachment 109949 [details] [diff] [review]
[real!] Eighteenth version: Fix bug + include updated t/*

I'm bored of this bug. r=gerv. If no-one else checks it in, I will in the next
couple of days. :-)

Gerv

Comment 66

[Gervase Markham [:gerv]]

Oh - we need approval first, don't we :-)

Gerv

Comment 67

[Gervase Markham [:gerv]]

Fixed.

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.110; previous revision: 1.109
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.218; previous revision: 1.217
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.2; previous revision: 1.1
done
Checking in t/004template.t;
/cvsroot/mozilla/webtools/bugzilla/t/004template.t,v  <--  004template.t
new revision: 1.24; previous revision: 1.23
done
Checking in t/005no_tabs.t;
/cvsroot/mozilla/webtools/bugzilla/t/005no_tabs.t,v  <--  005no_tabs.t
new revision: 1.12; previous revision: 1.11
done
Checking in t/Support/Templates.pm;
/cvsroot/mozilla/webtools/bugzilla/t/Support/Templates.pm,v  <--  Templates.pm
new revision: 1.12; previous revision: 1.11
done

Gerv

Comment 68

[Gervase Markham [:gerv]]

Oh - the change to 006spellcheck didn't apply, but it looked like the problem,
whatever it was, was already fixed.

Gerv

Comment 69

[Bradley Baetz (:bbaetz)]

Bleh. The my $var in the template isn't modperl safe. I suppose I should have
reviewed this earlier....

Don't worry about it, though; at some point someone (ie me) will need to audit
all this stuff anyway.

Comment 70

[Dave Miller [:justdave]]

We're going to wind up putting out a 2.16.3 anyway, because of various other
issues that have come up.  As long as we're doing it...  most of the template
sets that are starting to pop up are against the Bugzilla 2.16 branch, despite
the fact that you can't actually use them in 2.16.x without patching it.  We
might as well make life easy on everyone and make it official.

Tobias, I'm pretty sure you have a version of this patch for the 2.16 branch
already that you've been distributing with the bugzilla-de templates.  Any
chance we can get that contributed?  Does it have equivalent functionality, or
is it just a "make it work?"

Comment 71

[Dave Miller [:justdave]]

ok, on second thought, let's not.  the size of this patch is pretty huge, and
the stuff distributed with all the templates I've checked out now basically says
to hard-code it into your template search path and/or to apply the patch on this
bug (which won't apply to 2.16 cleanly - go figure :)

we seem to be leaning towards doing a 2.18 fairly soon anyway.