Bug 126955 (bz-l10n)

Bugzilla should support translated/localized templates

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Bugzilla-General
--
critical
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: Tobias Burnus, Assigned: Tobias Burnus)

Tracking

2.15
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 17 obsolete attachments)

20.80 KB, patch
gerv
: review+
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
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%).
(Assignee)

Updated

16 years ago
Blocks: 125584

Comment 1

16 years ago
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)
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.
per my last comment, moving this to 2.16 so we don't forget to deal with the
directory structure before release.
Target Milestone: --- → Bugzilla 2.16

Comment 4

16 years ago
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.

Updated

16 years ago
Blocks: 135604
bug 135707 is going to satisfy the 2.16 requirements for this.  Moving this bug
out to get it off the 2.16 list.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
(Assignee)

Comment 6

16 years ago
Created attachment 86613 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Keywords: patch, review
(Assignee)

Comment 7

16 years ago
Created attachment 86617 [details] [diff] [review]
Second version: Now searches custom, default instead of twice custom ;-)
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
(Assignee)

Comment 9

16 years ago
Created attachment 86926 [details] [diff] [review]
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.
(Assignee)

Comment 10

16 years ago
Created attachment 86927 [details] [diff] [review]
Fourth version: Fix 'use strict' error (add twice a 'my')
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
(Assignee)

Comment 12

16 years ago
> 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.
(Assignee)

Comment 13

16 years ago
Created attachment 86948 [details] [diff] [review]
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.
(Assignee)

Comment 14

16 years ago
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).
(Assignee)

Updated

16 years ago
Alias: bz-l10n

Comment 15

16 years ago
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. 
*** Bug 152556 has been marked as a duplicate of this bug. ***
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 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
Attachment #86948 - Flags: review+
(Assignee)

Comment 19

16 years ago
Created attachment 88318 [details] [diff] [review]
Sixth version: Updated param descriptions, use quotes in hash references
Attachment #86613 - Attachment is obsolete: true
Attachment #86617 - Attachment is obsolete: true
Attachment #86926 - Attachment is obsolete: true
Attachment #86927 - Attachment is obsolete: true
Attachment #86948 - Attachment is obsolete: true
(Assignee)

Comment 20

16 years ago
Created attachment 88320 [details] [diff] [review]
Seventh version: Fix '*' problem mentioned in comment 14
Attachment #88318 - Attachment is obsolete: true
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
(Assignee)

Comment 22

16 years ago
Created attachment 88445 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Attachment #88320 - Attachment is obsolete: true
Comment on attachment 88445 [details] [diff] [review]
Eighth version: Whitespace fixes, remove ($) prototyping

r=gerv.

Gerv
Attachment #88445 - Flags: review+
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 25

16 years ago
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.

Severity: normal → critical
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
'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.
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.
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
I'll revisit this next week. bbaetz: what are "those two"?

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

Gerv
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.
(Assignee)

Comment 32

16 years ago
> 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'.)
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)
> 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

(Assignee)

Comment 35

16 years ago
Created attachment 101093 [details] [diff] [review]
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/.
Attachment #88445 - Attachment is obsolete: true
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.
Attachment #101093 - Flags: review-
(Assignee)

Comment 37

16 years ago
Created attachment 102084 [details] [diff] [review]
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.
Attachment #101093 - Attachment is obsolete: true
(Assignee)

Comment 38

16 years ago
"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.
(Assignee)

Comment 39

16 years ago
Created attachment 102583 [details] [diff] [review]
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.
Attachment #102084 - Attachment is obsolete: true
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
Attachment #102583 - Flags: review+
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.
Attachment #102583 - Flags: review-
(Assignee)

Comment 42

16 years ago
Created attachment 102670 [details] [diff] [review]
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
Attachment #102583 - Attachment is obsolete: true
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
Attachment #102670 - Flags: review-
ping?
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".
(Assignee)

Comment 46

16 years ago
Created attachment 107031 [details] [diff] [review]
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 ;-)
Attachment #102670 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107031 - Flags: review?(bbaetz)
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  <zach@zachlipton.com>
> #                 Jacob Steenhagen <jake@bugzilla.org>
> #                 Bradley Baetz <bbaetz@student.usyd.edu.au>
>+#                 Tobias Burnus <burnus@net-b.de>
> #
> #
> # 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 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.
Attachment #107031 - Flags: review?(bbaetz) → review-
(Assignee)

Comment 49

16 years ago
Created attachment 107132 [details] [diff] [review]
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 ...]
Attachment #107031 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107132 - Flags: review?(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?
Attachment #107132 - Flags: review?(bbaetz) → review-
(Assignee)

Comment 51

16 years ago
Created attachment 107344 [details] [diff] [review]
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.
Attachment #107132 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107344 - Flags: review?(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.
Attachment #107344 - Flags: review?(bbaetz) → review-
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.
(Assignee)

Comment 54

16 years ago
Created attachment 107354 [details] [diff] [review]
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
Attachment #107344 - Attachment is obsolete: true
(Assignee)

Comment 55

16 years ago
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.)
Attachment #107354 - Flags: review?(zach)
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
(Assignee)

Comment 57

16 years ago
> 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

16 years ago
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...

Updated

16 years ago
Blocks: 182965

Updated

16 years ago
Blocks: 182975
(Assignee)

Updated

16 years ago
Attachment #107354 - Flags: review?(zach)
(Assignee)

Comment 59

16 years ago
Created attachment 109942 [details] [diff] [review]
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.
Attachment #107354 - Attachment is obsolete: true
(Assignee)

Comment 60

16 years ago
Created attachment 109949 [details] [diff] [review]
[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!))
Attachment #109942 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #109949 - Flags: review?(bbaetz)
(Assignee)

Updated

16 years ago
Attachment #109949 - Flags: review?(jake)
(I'll re-review this once bbaetz or jake is happy with it.)

Gerv
(Assignee)

Comment 62

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

Comment 63

16 years ago
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
Attachment #109949 - Flags: review?(gerv)

Comment 64

16 years ago
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...
Assignee: justdave → burnus
Status: REOPENED → NEW
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
Attachment #109949 - Flags: review?(gerv) → review+
Oh - we need approval first, don't we :-)

Gerv
Flags: approval?
Attachment #109949 - Flags: review?(jake)
Attachment #109949 - Flags: review?(bbaetz)
Flags: approval? → approval+
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
Oh - the change to 006spellcheck didn't apply, but it looked like the problem,
whatever it was, was already fixed.

Gerv
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.
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?"
Whiteboard: [wanted for 2.16.3]
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.
Whiteboard: [wanted for 2.16.3]
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.