You are my template, my only template, you keep my happy when skys are grey... Gerv
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Keywords: patch, review
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?
Created attachment 67597 [details] [diff] [review] Patch v.2 Updated version with =afranke. Gerv
Attachment #63125 - Attachment is obsolete: true
Created attachment 67884 [details] [diff] [review] 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 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 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/perl188.8.131.52/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? :-)
>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.
Strange, I'm pretty sure I did mark my patch needs-work, but in the activity log it shows up as kiko. Known bug?
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
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.
Created attachment 68598 [details] [diff] [review] 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
>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 on attachment 68598 [details] [diff] [review] Patch v.3 2r=kiko and marking r=afranke. Looks good for check-in.
Attachment #68598 - Flags: review+
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
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.