117515 - Templatise describekeywords.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Gervase Markham [:gerv]]

You are my template, my only template, you keep my happy when skys are grey...

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This one was pretty easy.

Gerv

Comment 2

[Andreas Franke (gone)]

I've taken a look at this, and it's all plausible. r=afranke if that helps.

However, I've tried to apply it, and my patch program rejected the first hunk;
no idea why. 

Another thing: you are using an "html" filter for the keyword in the url;
shouldn't this be a "uri" filter or something?

And I think I've seen a bug with a patch to CGI.pl that introduced a global
template object; maybe the patch can be simplified a bit if that one lands
before this one?

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Updated version with =afranke.

Gerv

Comment 4

[Andreas Franke (gone)]

Attachment: cvs diff -u describekeywords.cgi , using $::template and $::vars (v.3)

This is like it's done in enter_bug.cgi. 

Since I was unable to produce a combined patch with describe_keywords.tmpl,
I will upload it separately, even though it's the same as in Patch v.2.

Comment 5

[Andreas Franke (gone)]

Comment on attachment 67884 [details] [diff] [review]
cvs diff -u describekeywords.cgi , using $::template and $::vars (v.3)

Grrr, I think I'll leave it to Gerv to attach patches. This patch changes the
path to perl, so marking needs-work. :-(

Comment 6

[Christian Reis]

Comment on attachment 67884 [details] [diff] [review]
cvs diff -u describekeywords.cgi , using $::template and $::vars (v.3)

I have some comments here, but it looks in general ready to go.
If these are fixed, r=kiko.

>--- describekeywords.cgi	2002/01/20 01:44:37	1.6
>+++ describekeywords.cgi	2002/02/05 10:56:22
>@@ -1,4 +1,4 @@
>-#!/usr/bonsaitools/bin/perl -wT
>+#!/opt/perl5.0.0.4/bin/perl -wT

Eeek. :-)

>+   
>+    # This 'onebug' stuff is silly hackery for old versions of
>+    # MySQL that seem to return a count() of 1 even if there are
>+    # no matching. So, we ask for an actual bug number. If it

s/matching/matches/

Can we drop this now that we require a modern version of MySQL as
per checksetup.pl?

>+print "Content-type: text/html\n\n";
>+$template->process("info/describe_keywords.tmpl", $vars)
>+  || DisplayError("Template process failed: " . $template->error())
>+  && exit;


I think we decided on .html.tmpl, right? :-)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

>Can we drop this now that we require a modern version of MySQL as
>per checksetup.pl?

Yes, it looks like this bug was fixed in 3.21.22, and we require 3.22.5.


>I think we decided on .html.tmpl, right? :-)

True for templates with formats, but otherwise we didn't decide.

Comment 8

[Andreas Franke (gone)]

Strange, I'm pretty sure I did mark my patch needs-work, but in the activity log
it shows up as kiko. Known bug?

Comment 9

[Gervase Markham [:gerv]]

We should go for .html.tmpl in all cases, I think, because then templates can
acquire formats with minimal fuss. Myk - is that reasonable?

(/me sees us renaming all the templates before release...)

Gerv

Comment 10

[Andreas Franke (gone)]

Another possibility: If you are looking for an html template, and you can't find
a .html.tmpl, then look for a .tmpl also. But this may be a bit slower. 

Having everything as .html.tmpl is probably the cleanest solution.

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Try this. Uses $::templates and $::vars, the ancient cruft is removed, and the
template is now called describe-keywords.html.tmpl , in line with the latest
thinking on file naming.

Gerv

Comment 12

[Myk Melez [:myk] [@mykmelez]]

>We should go for .html.tmpl in all cases, I think, because then templates can
>acquire formats with minimal fuss. Myk - is that reasonable?

Sounds good to me.

>(/me sees us renaming all the templates before release...)

Yes, we have to do this anyway for the .atml templates, so we should bundle all
the changes together and do them all at once (and wait until the end in case
some other change comes up).

Comment 13

[Christian Reis]

Comment on attachment 68598 [details] [diff] [review]
Patch v.3

2r=kiko and marking r=afranke. Looks good for check-in.

Comment 14

[Gervase Markham [:gerv]]

Checking in describekeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/describekeywords.cgi,v  <--  describekeywords.cgi
new revision: 1.7; previous revision: 1.6
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/info/describe-keywords.html.tmpl,v
done
Checking in template/default/info/describe-keywords.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/info/describe-keywords.html.tmpl,v
 <--  describe-keywords.html.tmpl
initial revision: 1.1
done

Gerv