143650 - General template display system

Status: RESOLVED FIXED

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

Description

[Gervase Markham [:gerv]]

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.

http://bugzilla.mozilla.org/display.cgi?page=account-policy

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.)

Gerv

Comment 1

[Gervase Markham [:gerv]]

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.

Gerv

Comment 2

[Gervase Markham [:gerv]]

CCing more people for advice/comments.

Gerv

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.1

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

Gerv

Comment 4

[Jouni Heikniemi]

How about "displayinfo.cgi"?

Comment 5

[Gervase Markham [:gerv]]

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

Gerv

Comment 6

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

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:

http://foo.bar.com/display.cgi/path/to/filename.html
http://foo.bar.com/display/path/to/filename.html (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.

Comment 7

[Gervase Markham [:gerv]]

How about e.g.:

http://bugzilla.mozilla.org/page/why-account

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.

Gerv

Comment 8

[Dave Miller [:justdave]]

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).

Comment 9

[Jouni Heikniemi]

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

Comment 10

[Gervase Markham [:gerv]]

Well, that sounded convincing :-)

OK:
http://bugzilla.mozilla.org/page.cgi?id=why-account
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?

Gerv

Comment 11

[Jouni Heikniemi]

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
mapping. 

Comment 12

[Gervase Markham [:gerv]]

localconfig it is, then. I'll implement that when I get round to it.

Gerv

Comment 13

[Gervase Markham [:gerv]]

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

Gerv

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.2

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 checksetup.pl
will be empty.

The syntax is:
http://bugzilla.mycompany.com/page.cgi?id=why-account

Gerv

Comment 15

[Jouni Heikniemi]

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. 
2xr=jouni

Comment 16

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Here's what I actually checked in :-) The single space removal is for
consistency within the file.

Gerv

Comment 17

[Gervase Markham [:gerv]]

Checked in.

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

Gerv