General template display system

RESOLVED FIXED in Bugzilla 2.18

Status

()

P3
critical
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: gerv, Assigned: gerv)

Tracking

2.15
Bugzilla 2.18

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
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
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
CCing more people for advice/comments.

Gerv
Created attachment 83187 [details] [diff] [review]
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

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

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

17 years ago
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 :-)

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

17 years ago
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. 
localconfig it is, then. I'll implement that when I get round to it.

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

Gerv
Severity: normal → critical
Created attachment 92140 [details] [diff] [review]
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
Attachment #83187 - Attachment is obsolete: true

Comment 15

17 years ago
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
Attachment #92140 - Flags: review+
Created attachment 92290 [details] [diff] [review]
Patch v.3

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

Gerv
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
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.163; previous revision: 1.162
done

Gerv
Status: ASSIGNED → RESOLVED
Last Resolved: 17 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.