696440 - Use WRAPPER in Bugzilla::Template->create to include the header and footer of HTML pages

Status: NEW

(Bugzilla :: User Interface, enhancement)

Description

[Frédéric Buclin]

Currently, our templates are full of [% PROCESS global/header.html.tmpl %] and [% PROCESS global/footer.html.tmpl %] to include the header and footer of HTML pages. In some cases, this is problematic because what should be displayed in the header is not yet known when header.html.tmpl is called, or calling the header/footer templates depends if the template has already been called or not (e.g. in mass-change where we loop over each bug). To remove some useless code from our templates and to remove some of the hacks introduced in them, we should use the WRAPPER option in Bugzilla::Template->create(). This has no noticeable benefit on performance; this only slightly decreases the number of calls in templates.

Comment 1

[Frédéric Buclin]

More info: the wrapper is called last, and can access all variables defined in the already processed templates. So this lets us e.g. set the title and JS + CSS scripts to load in the header very late in the process, right before displaying the page. For non-HTML pages, we just have to pass no_wrapper => 1 to Bugzilla->template and that's it.

Comment 2

[Frédéric Buclin]

Attachment: Proof of concept, v0.01

Here is a proof of concept. It works with show_bug.cgi and some admin pages + a page or two I played with. I forgot to include global/wrapper.html.tmpl in the patch. It simply contains:

[% PROCESS global/header.html.tmpl %]
[% content %]
[% PROCESS global/footer.html.tmpl %]

I suppose that at this point, we could merge both the header and footer templates into a single one, to avoid two useless PROCESS calls.

Comment 3

[Max Kanat-Alexander]

Oh, this will not work for process_bug.cgi, which displays the header separately in a separate template.

We also can't put this into create's WRAPPER, because not all of our pages are HTML.

I do generally like the idea of using WRAPPER where we can, if it makes the templates simpler.

Comment 4

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #3)
> Oh, this will not work for process_bug.cgi, which displays the header
> separately in a separate template.

I already thought about that, same for non-HTML pages. And I think I have a reasonable solution. :) You can enclose the header and banner into

[% UNLESS no_header %]
  [% PROCESS global/header.html.tmpl %]
[% END %]

[% content %]

[% UNLESS no_footer %]
  [% PROCESS global/footer.html.tmpl %]
[% END %]

Displaying raw attachments won't be affected as we don't use $template->process at all.

Comment 5

[Frédéric Buclin]

I did very good progress on this, so I will do it myself. process_bug.cgi (including mass-change) is working fine now, and even viewing patches in diff mode works great (I had to work around the way PatchReader works, but it appeared it was in fact easy to do).

Comment 6

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #4)
> I already thought about that, same for non-HTML pages. And I think I have a
> reasonable solution. :) You can enclose the header and banner into [snip]

  Sure, that sounds possible! :-) Perhaps we could also analyze the format of the processed template, and not run the wrapper at all if it's not .html?

  If this simplifies all of our templates, that would be pretty awesome.

Comment 7

[Frédéric Buclin]

Attachment: WIP, v0.5

This WIP patch is not complete (global/wrapper.html.tmpl is not included in the patch + it needs more testing), but it shows how the code would look like. It brings very little value (the code cleanup is small, and there is no perf win), and so I stopped working on it a few days after I filed this bug. I'm attaching it here anyway in case someone else wants to finish the work. But it's low priority and I have more important work to do.