Closed Bug 143650 Opened 22 years ago Closed 22 years ago

General template display system


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




Bugzilla 2.18


(Reporter: gerv, Assigned: gerv)



(1 file, 2 obsolete files)

I have a need to serve a mostly-static page, but I want it to have Bugzilla
headers and footers. I anticipate that other Bugzilla installations may want to
do this - for policy documents, or other information.

Bugzilla should have a generic template display CGI, which takes a single
parameter, being a tag for the template to display.

display.cgi would quietly_check_login, and then display the template which
corresponds to the tag "account-policy". This template would get anything that
is always added to $vars (such as user_agent) and would be able to add the
Bugzilla header and footer.

(Other implementation options include
- the page parameter being a full template path; but this allows people to
display arbitrary templates, so is probably a bit insecure
- the page parameter being a full template name, and the templates always being
in one particular directory. This avoids the above problem, but may stop you
putting your template with associated other templates in the filesystem.)

I want to put a page up on b.m.o, explaining why we require people to get
accounts before submitting bugs. This CGI is the generic mechanism for doing that.

Related: bug 133559.

Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
CCing more people for advice/comments.

Attached patch Patch v.1 (obsolete) — Splinter Review
New display.cgi. I am looking for feedback on:

- whether this is a good idea
- the name of the CGI
- the mechanism for mapping tags to templates
- any other info we should provide to the template via this CGI

How about "displayinfo.cgi"?
Well, I don't want to unnecessarily reduce its usefulness by making the name too
specific. But that might be good...

This is a good idea, and this system could probably also be used for
documentation and other pages that are currently a mix of static HTML pages
(some of which should be slightly dynamic) and mostly-static CGIs like
confirmhelp.html and queryhelp.cgi.

I suggest making the URLs look like requests for actual pages, however, because
they are easier to remember and recognize, which is useful for "static" pages
that don't take parameters.  Either of these two styles of calling a CGI script
makes this possible: (Where "display" is actually a
CGI script.)

Obviously there would have to be a mechanism for converting "filename.html" to a
template filename (possibly just adding ".tmpl" to it).  "display.cgi" is ok,
but "displayinfo.cgi" seems unnecessarily long.  Something even simpler like
"page.cgi" would be even better.
How about e.g.:

We should take the opportunity to avoid specifying in the URL what the type of
the file is. And I think we should have a mapping from tags ("why-account") to
templates ("template/accounts/why-account.html.tmpl") in the CGI or an external
file, to prevent people using it to run arbitrary templates, which I'm sure
could lead to some obscure problems and security issues.

The problem with not putting a .cgi or .pl extension on the script is leaving it
extensionless requires cooperation from the webserver config to make it run as a
script.  If you have Apache, this is pretty painless (add <Files "page">
SetHandler cgi-script </Files> to your .htaccess for that directory), but I'm
not sure it's so easy on other webservers (IIS in particular).
That server/cgi-name/parameters/go/here URL schema is one of the worst
inventions around and should not be used in Bugzilla. The
display(info).cgi?page=foo is just fine. But some arguments, then:

- While IIS can run cgi's with the mentioned syntax, this is not without
  problems and some limitations.
- While IIS and Apache can cope with it, not all the web servers in the 
  world can.
- It makes using relative links a pain, since the page template just can't
  say <img src="image.gif">.
- It messes up the URI vs. directory structure mapping
- It requires explicit configuration on most servers
Well, that sounded convincing :-)

will do fine, then.

The other question is: where do we put the id -> template mapping? Currently,
it's in the CGI file itself, which isn't ideal. It could be in a meta-template.
Or does anyone have a better idea?

I must say I'm becoming worried about all the uses templates are getting.
Sinking the plugin program logic (in bug 142101) into them is quite harsh
already, but stuffing configuration into templates? Ummm... Additionally, this
reminds me of a certain Gerv telling me "Every time we've stored information
outside the DB for something, we've regretted it later" in bug 142736. ;---) 

In this context, I think the current configuration files are "database" as well.
Nevertheless, my point is that we already have administrator configurable
options in params, localconfig and some source files (see bug 144177 for an
example). I wouldn't want to configure yet another file. 

So I propose a new hash in localconfig, since this is definitely a "file-level"
setting (see bug 144177 comment 3) and changing the templates requires
filesystem access anyway so you don't need a UI for configuring the id->template
localconfig it is, then. I'll implement that when I get round to it.

Changing severity to indicate this bug's importance. (Actually, I'm demoing

Severity: normal → critical
Attached patch Patch v.2 (obsolete) — Splinter Review
OK, it now uses localconfig.

The why-account page is included for the reviewer to use testing purposes; it
is not meant for checkin. So, when checked in, the %pages hash in
will be empty.

The syntax is:

Attachment #83187 - Attachment is obsolete: true
Comment on attachment 92140 [details] [diff] [review]
Patch v.2

>+# then call page.cgi?page=<tag> . Tags may only contain [\w-].

This display system is a tool that could well be used by less-experienced
sysadmins, so its comments should be written appropriately. Please expand the
regexp to something like "letters a-z, numbers 0-9 and the hyphen (-)". It
doesn't have to be exact, but it has to be understandable.

>+$::FORM{'id'} =~ s/[^\w-]//g;

Check this for undef first to avoid a warning if the user just loads "page.cgi"
without query parameters. 

>+    $template->process("$pages{$::FORM{'id'}}", $vars)

Unless there's particular reason to use string interpolation above, zap the
unnecessary double quotes.

>@@ -523,7 +523,6 @@
> LocalVar('contenttypes', '

What's that got to do with a generic template display system? 

Fix those and attach the final patch, please. 
Attachment #92140 - Flags: review+
Attached patch Patch v.3Splinter Review
Here's what I actually checked in :-) The single space removal is for
consistency within the file.

Attachment #92140 - Attachment is obsolete: true
Checked in.

Checking in page.cgi;
/cvsroot/mozilla/webtools/bugzilla/page.cgi,v  <--  page.cgi
initial revision: 1.1
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.163; previous revision: 1.162

Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.