229658 - mechanism for extensions to hook into templates

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

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

Bugzilla extensions sometimes need to add elements to existing UI, so we need a
way to add hooks into templates that can call out to extension templates without
knowing what extensions are installed in advance.

One way is to create a series of directories, one for each place an extension
might want to add elements, and process any templates found in those directories
whenever the corresponding Bugzilla template is processed.

For example, if we wanted to put a hook in globals/useful-links.html.tmpl for
adding links to the list of admin CGIs, we could create the
template/en/default/hooks directory, create the hooks/useful-links and
hooks/useful-links/admin subdirectories, and then put some code in the
appropriate place in that template for processing any templates found in that
subdirectory.

I've cooked up a patch I'll attach shortly.  Per my patch and the example above,
here's the code that would need to be added to useful-links.html.tmpl:

[% PROCESS $hook FOREACH hook = GetHooks("hooks/useful-links/admin") %]

The hook directories and the hooks that use them can be created on an as-needed
basis, but we should require that people file a bug and ask for review so we
have some oversight over where they get created (like with other template
directories).  We should do something like this for code as well.

Comment 1

[Dave Miller [:justdave]]

I love this idea.  It would help the P4DTI folks, too, since we've already
attempted to get hooks for them in in a couple places.

As for that specific instance (useful links admin section), I'm starting to
dislike having links for all of the admin pages in the footer.  It's growing to
the point of looking unweildly if you have all admin privs :)  What they did on
Zippy's Bugzilla is actually starting to grow on me...  those links are simply
"Preferences, Project Administration, System Administration".  Folks with
editcomponents, editkeywords, etc would get the Project Admin link, and folks
with tweakparams, editgroups, editusers, and so forth would get the System Admin
link.  The CGIs are all linked together with a tab bar across the top to pick
which thing you want to go in to (groups, users, sanitycheck, etc).

Comment 2

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

Attachment: patch v1: implements GetHooks() function for generating list of files in given directory

Comment 3

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

Comment on attachment 138117 [details] [diff] [review]
patch v1: implements GetHooks() function for generating list of files in given directory

bbaetz- Can you review this small patch (if not code, then at least overall
approach)?

Comment 4

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

Comment on attachment 138117 [details] [diff] [review]
patch v1: implements GetHooks() function for generating list of files in given directory

Dave- agreed on the UI issues.	The footer is getting unwieldy.  This is just
an example, though, of course; the principle applies to just about anywhere in
templates.

Do you have time to give this small patch the once-over?

Comment 5

[Gervase Markham [:gerv]]

> [% PROCESS $hook FOREACH hook = GetHooks("hooks/useful-links/admin") %]

A few thoughts:

- The "hooks" in the GetHooks call is probably redundant. 
- Is "GetHooks" (studlycaps, initial cap) consistent with our other 
  template-exposed funcs?
- Do we want to mandate that the directory structure under the hooks directory
matches the directory structure under the default directory? Can we auto-detect
hook names in this way, if a template knows its own path?

[% PROCESS $hook FOREACH hook = GetHooks("1.html.tmpl") %]

in template/en/default/global/useful-links.html.tmpl would get
hooks/global/useful-links/1.html.tmpl and include it.

Would something like that be sensible?

- Do we need a mechanism for managing different content-types? What if you
wanted a .none.tmpl hook inside a .html.tmpl file?

Gerv

Comment 6

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

>- The "hooks" in the GetHooks call is probably redundant. 

Right.


>- Is "GetHooks" (studlycaps, initial cap) consistent with our other
>  template-exposed funcs?

The others are Param, time2str, PerformSubsts, lsearch, UserInGroup,
SendBugMail, and SyncAnyPendingShadowChanges, so it's consistent with the most
common style.


>- Do we want to mandate that the directory structure under the hooks
>  directory matches the directory structure under the default directory?
>  Can we auto-detect hook names in this way, if a template knows its own path?

Seems reasonable, whether or not templates can know their own path, although the
path itself is insufficient since some templates will want multiple hooks (f.e.
useful-links, which wants one to add links to the list of links for everyone and
one to add links to the list of administration links), each of which needs to be
separated into separate directories (f.e. useful-links/general/ and
useful-links/admin/).

>[% PROCESS $hook FOREACH hook = GetHooks("1.html.tmpl") %]
>
>in template/en/default/global/useful-links.html.tmpl would get
>hooks/global/useful-links/1.html.tmpl and include it.
>
>Would something like that be sensible?

Almost, except for the part where a specific template is referenced in the hook.
 We don't want to have to determine in advance which extensions a user might
install (and thus which specific extension templates to reference in the hooks).

Thus, instead we create a directory for each hook (f.e.
hooks/global/useful-links/), let extensions place whatever templates they want
into it (f.e. hooks/global/useful-links/link-to-my-super-extension.html.tmpl),
and parse all template files in that directory when the template containing the
hook gets processed.

>- Do we need a mechanism for managing different content-types? What if you
>  wanted a .none.tmpl hook inside a .html.tmpl file?

Good point.  Probably the thing to do is pass GetHooks the Bugzilla template's
content type and have it only process extension templates of the same content
type.  Alternatively we could create separate directories for hooks of different
content types, but that seems unnecessary.

Comment 7

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

Attachment: patch v2: addresses gerv's suggestions

This patch auto-detects the template path (although it's buggy without a TT
fix) and looks in INCLUDE_PATH/hooks/template-path-and-name/hook-name/, for
example if we added an "admin-links" hook to useful-links.html.tmpl via the
following call:

[% PROCESS $hook FOREACH hook = GetHooks("admin-links") %]

then we'd process hook files in:

(custom|default)/hooks/global/useful-links.html.tmpl/admin-links/

So all templates have their own directory for hook files with subdirectories as
needed for multiple hooks, and hooks for different content types and formats of
the "same" template (i.e. useful-links.xml.tmpl or list/list-simple.html.tmpl)
are in different directories (which makes sense since those hooks are unlikely
to be shareable).

Comment 8

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

Comment on attachment 138348 [details] [diff] [review]
patch v2: addresses gerv's suggestions

Gerv, does this version work the way you'd expect?

Comment 9

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

Attachment: patch for Template Toolkit

Template Toolkit has a bug that will cause GetHooks to return the wrong list of
hooks sometimes.  This is the fix.  I've already submitted this to Andy via the
mailing list; it's a simple fix for an obvious problem and should make it into
the next release.

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 138348 [details] [diff] [review]
patch v2: addresses gerv's suggestions

You can't use $::template because of mod perl. Isn't the context passed in as
an arg to the sub anyway? Why doesn't the globbing trigger taint errors?

why does this need to be part of a template function, and how does this work
with a flexible datadir?

Comment 11

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

Attachment: patch v3: switches to macro to avoid $::template

>You can't use $::template because of mod perl. Isn't the context passed in as
>an arg to the sub anyway?

Unfortunately not.  Filters, macros, and plugins all get a context object, but
regular variables that happen to be functions don't.  After playing around with
various approaches it looks like a Perl code macro is the best option for
avoiding $::template, so I moved the code from the VARIABLES list to a template
defining the macro and use PRE_PROCESS to make the macro available to all
templates.


>Why doesn't the globbing trigger taint errors?

Because I detaint while removing the INCLUDE_PATH from the hook path.


>why does this need to be part of a template function,

It's not important that it be a template function, just that it be callable
from anywhere in the templates.


>and how does this work with a flexible datadir?

It should work fine since it works a level above the datadir, getting a list of
original template paths from TT and using TT's standard mechanisms for
processing templates (which handle all issues of compilation, datadir, etc.). 
How are we using flexible datadirs?

Comment 12

[Gervase Markham [:gerv]]

I've been thinking about this: I'll try and write up my thoughts this evening.
(I'm at work now.)

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

But the templates don't want to find everything from a certain directory. What
they want to do is process all the hooks. Thats what should be the function, not
this. Isn't this better off as a plugin of some sort?

> How are we using flexible datadirs?

Its a var in Bugzilla::Config, mostly for mod_perl use.

Comment 14

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

>But the templates don't want to find everything from a certain directory. What
>they want to do is process all the hooks. Thats what should be the function,
>not this.

Maybe I don't understand what you're saying, because what you just described
templates wanting to do is exactly what this function does: it finds and
processes all the hooks for a given location in a template.

True, it does so by finding and processing all files in a given directory, but
that's because we're using unique directories to store the hooks for each
location in each template, and the reason for that it that it makes it possible
for extensions to install hooks without some complex scheme for registering them
with Bugzilla or modifying templates to process specific hook files.

Instead, extensions place hooks into the appropriate directories, templates
process any hooks in the appropriate directories, and everything just works.


>Isn't this better off as a plugin of some sort?

Perhaps, although making it a plugin is more complicated, and the only advantage
I can see is that we don't have to turn on EVAL_PERL.  We would still want to
PRE_PROCESS some template that USEs the plugin, since most templates will want
to be able to process hooks (if only to let extensions add navigation UI like in
my useful-links example).

Comment 15

[Bradley Baetz (:bbaetz)]

"Abstraction". I guess that we could install PROCESS a template which called
that function, but I don;t want your FOREACH thing scattered around the place.

Comment 16

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

>"Abstraction". I guess that we could install PROCESS a template which called
>that function, but I don;t want your FOREACH thing scattered around the place.

Take another look at the latest patch I attached.  That's exactly what I do.

Comment 17

[Gervase Markham [:gerv]]

Further thoughts:

- We have several templates called e.g. "create.html.tmpl". I think the right
directory structure for this, taking into account the above fact and customised
templates which may still require hooks, is:

- template
  |- en
     |- default
        |- global
        |- admin
        |- ...
     |- custom
        |- global
        |- admin
        |- ...
     |- extension
        |- global
        |- admin
        |- ...

So the "hook" directory gets called "extension" (because that's what is actually
in it - the hooks are in the original template files), and the directory
structure below it mirrors exactly the directory structure below default, except
that (as is the current plan) each template name is, in fact, a directory, and
it contains all the extension files for that template.

- I'm not 100% sure what the in-template syntax of Myk's current patch is but
IMO, the sort of syntax we want is (in order of idealness):

[% HOOK(3) %]
[% HOOK index = 3 %]
[% ProcessHooks(3) %]
[% PROCESS HOOK index = 3 %]

All of these would PROCESS
template/en/extension/path/to/template.html.tmpl/*3.html.tmpl
at that particular point in the template. The * (wildcard) allows all extension
templates to have an extension-specific prefix, which means they can be easily
identified and distinguished, and multiple extensions using the same hook don't
clash.

- We should mandate that each hook is preceded by a template comment explaining
what it is there for. This allows us to put it in the right place if the
template ever gets rearranged.

- Does turning on EVAL_PERL:
  - have an effect on performance?
  - raise security issues?

Gerv

Comment 18

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

>- We have several templates called e.g. "create.html.tmpl". I think the right
>directory structure for this, taking into account the above fact and
>customised templates which may still require hooks, is [an extension
>directory at the same level as the custom and default directories.]

The problem with that location is that it makes it impossible to customize an
extension by overriding it with a custom template.  Extensions are likely to
need customization as much as standard Bugzilla templates do, and it should be
possible for installations to use custom templates for that purpose when useful,
which is why it makes sense for the extensions directory to be inside the
default directory, not next to it.


>So the "hook" directory gets called "extension" (because that's what is
>actually in it - the hooks are in the original template files)

Right, that makes sense.


>- I'm not 100% sure what the in-template syntax of Myk's current patch is but
>IMO, the sort of syntax we want is (in order of idealness):

My goals for the syntax are simplicity (preferably single-line constructs) and
clarity.  Anything all-caps like HOOK is discouraged by TT because of potential
conflicts with future reserved words.  That leaves only "[% ProcessHooks(3) %]"
from your list, which is basically what the current patch does, except I rename
it to process_hooks because there's no precedent yet for macro naming and other
parts of our code have been migrating to that style.  Given your point about
hooks vs. extensions, however, I'll change this to process_extensions.


>All of these would PROCESS
>template/en/extension/path/to/template.html.tmpl/*3.html.tmpl

Instead of a filename component, the current patch uses a hook-specific
subdirectory of the template extension directory, i.e.:

template/en/extension/path/to/template.html.tmpl/3/*.tmpl

Subdirectories have a couple advantages over filename components:

First, the template filename space is already quite crowded and complicated. 
For extensions it consists of at least 1. an extension-specific prefix, 2. a
name, 3. a file extension, and 4. a template extension (i.e.
prefix-name.ext.tmpl).  Adding a hook identifier component to the filename (i.e.
prefix-name-hookid.ext.tmpl) overburdens and overcomplicates the filename, while
identifying the hook via a subdirectory (i.e. hookid/pfx-name.ext.tmpl) does not.

Second, if we put hook identifiers into the filename we'll want them to be brief
(i.e. the numeral in your example above) to minimize their burden, but that
makes them obtuse.  Subdirectory names, on the other hand, can be descriptive,
thus self-documenting their purpose.  They can also be created in advance,
making it easier for extension authors to discover them just by browsing the
default/extension directory.  Filename components, on the other hand, give no
such clue unless the author has already installed an extension that extends the
same template.

For example, if you're looking for a way to extend bug/edit.html.tmpl you are
much more likely to understand [% process_hooks("addl-form-fields") %] and [%
process_hooks("addl-knobs") %] than [% process_hooks(3) %] and [%
process_hooks(4) %].  In fact, you may be able to deduce the right place to put
your extension just by examining the subdirectories in default/extension and
noticing default/extension/bug/edit.html.tmpl/addl-form-fields/ and addl-knobs/.


>- We should mandate that each hook is preceded by a template comment explaining
>what it is there for. This allows us to put it in the right place if the
>template ever gets rearranged.

If we use good, concise, but descriptive hook names we shouldn't need to provide
additional documentation.  Hooks should be very simple and unobtrusive, easily
located by extension authors via a standard file search but minimally
distracting for Bugzilla developers.  Ideally we want them on a single line.

>- Does turning on EVAL_PERL:
>  - have an effect on performance?
>  - raise security issues?

TT doesn't do any double-interpolation that I know of, so it shouldn't be
possible for remote users to insert Perl into a template and have it be
processed.  For local users it's as secure as the templates themselves.  It does
give customizers more rope to hang themselves, but they have to know the rope
exists to use it.

As for performance, EVAL_PERL itself doesn't cost anything (it merely tells TT
to stop ignoring PERL and RAWPERL blocks), although PRE_PROCESSing the macro
will cost something, and the work itself costs some small amount.

Comment 19

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

Attachment: patch v4: macro -> plugin and hook -> extension

This patch switches to using a plugin rather than a macro and an "extension"
directory rather than a "hooks" directory.  This means no more EVAL_PERL, so we
don't have to worry about performance or security issues there, plus a syntax
change from [% process_extensions(hook) %] to [% Extension.process(hook) %].

Otherwise things stay the same per my previous comment.

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 138517 [details] [diff] [review]
patch v4: macro -> plugin and hook -> extension

OK then.

Comment 21

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

>- We have several templates called e.g. "create.html.tmpl". I think the right
>directory structure for this, taking into account the above fact and
>customised templates which may still require hooks, is [an extension
>directory at the same level as the custom and default directories.]

Note that extensions will want to install their own primary templates in
addition to sub-templates processed by hooks.  If we wanted to separate those
primary templates into their own space (as opposed to having authors just put
them in the "default" directory) we could implement an "extension" directory
like you suggest at the same level as the "custom" and "default" directories and
include it in the INCLUDE_PATH between "custom" and "default".

In other words, the INCLUDE_PATH would be "custom, extension, default" so that
extensions could install themselves into a separate directory that would
override default templates (when necessary, which should be rarely, as we should
discourage this practice just as we do custom templates as an installation
maintenance headache) but could still be overridden themselves by
installation-specific customizations in the "custom" directory.

If we did that we could then put sub-templates processed by hooks into a
subdirectory of this same-level "extension" directory instead of sticking them
into a subdirectory of the "default" directory.  We'd want to name this
subdirectory something other than "extension", though, since
"extension/extension" is obtuse, referring to two different things with the same
word.

I've been reading references to  programming hooks on the web, and it looks like
 the term "hook" is commonly used not only for the point in a program at which
procedures extending the program are called but also for the procedures
themselves.  Other terms in use like "procedure" and "subroutine" tend to be too
general and programming-oriented for templates, so I suggest we name this
directory "extension/hook".

I'm open to alternatives, though, including ones in which we replace "extension"
by something like "addon" so we can use "extension" for the hook directory,
although I think "extension" is really best used to refer to the overall
extension, not just its hooks.  Thoughts?

Comment 22

[Gervase Markham [:gerv]]

So the distinction would be:

- default:   Templates shipped with Bugzilla
- extension: Packaged Bugzilla extensions, such as P4DTI
- custom:    installation-specific overrides to templates in either of the above

?

I like this. And we could have both an extension/hook directory, for hooks which
come with extensions, and a custom/hook directory, for custom hooks - further
reducing the need to mess with the default templates. Can you change your code
to look in both places?

The content-type of hooks is rather defined by the content type of the template
they are hooking into (and so the directory name they are in) - but, as you have
correctly coded, I think we should still make them end in .tmpl.

One note: hooks should be PROCESSed rather than INCLUDEd, so they can affect
template variables in the calling template. You appear to be doing this, but I
thought I'd state it explicitly.

Gerv

Comment 23

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

Attachment: patch v5: adds extension directory to INCLUDE_PATH

Ok, updated the patch with the new terminology and addition of "extension"
directory next to "custom" and "default".  Changed Extension.pm to Hook.pm per
the new terminology.  Added the necessary code to checksetup.pl and
t/004template.pl.


>So the distinction would be:
>
>- default:   Templates shipped with Bugzilla
>- extension: Packaged Bugzilla extensions, such as P4DTI
>- custom:    installation-specific overrides to templates in either of the 
>above

Right.


>And we could have both an extension/hook directory, for hooks which
>come with extensions, and a custom/hook directory, for custom hooks - 
>further reducing the need to mess with the default templates.

Yeah, good point, customizers can use hooks just as effectively as extenders.


>Can you change your code to look in both places?

The code already scans the INCLUDE_PATH, so it'll pick up anything in custom
automatically.

Note: TT 2.12 (out today or tomorrow) will contain my bug fix in attachment
138349 [details] [diff] [review].

Comment 24

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

Attachment: patch v6: removes unnecessary EVAL_PERL in checksetup.pl

The previous patch included a now-unnecessary EVAL_PERL in checksetup.pl. 
Removed.

Comment 25

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

Comment on attachment 138601 [details] [diff] [review]
patch v6: removes unnecessary EVAL_PERL in checksetup.pl

Gerv, can you review?  There have been no major changes in bbaetz' issues since
the version he reviewed, so presuming his review stands for his issues. 
bbaetz- tell me if that's not the case.

Comment 26

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

Comment on attachment 138601 [details] [diff] [review]
patch v6: removes unnecessary EVAL_PERL in checksetup.pl

Argh, I keep forgetting to include files.

Comment 27

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

Attachment: patch v7: includes t/Support/Templates.pm

Comment 28

[Gervase Markham [:gerv]]

Comment on attachment 138602 [details] [diff] [review]
patch v7: includes t/Support/Templates.pm

I should really hold review until the patch includes a documentation update too
;-) I'll just have to ask you to write one.

r=gerv.

I do think the seven iterations of this patch were worth it. The solution we've
come up with is pretty cool, IMO.

Gerv

Comment 29

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

>I should really hold review until the patch includes a documentation update
>too ;-) I'll just have to ask you to write one.

Yes, please file a bug on me and nag me until I do it. :-)


>I do think the seven iterations of this patch were worth it. The solution
>we've come up with is pretty cool, IMO.

Yeah, I agree.  Discussions in this bug were constructive and productive,
resulting in significant improvement to the final product.  A great example of
open source at work.

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.14; previous revision: 1.13
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/initialize.none.tmpl,v
done
Checking in template/en/default/global/initialize.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/initialize.none.tmpl,v
 <--  initialize.none.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v
done
Checking in Bugzilla/Template/Plugin/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v  <--  Hook.pm
initial revision: 1.1
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.257; previous revision: 1.256
done
Checking in t/004template.t;
/cvsroot/mozilla/webtools/bugzilla/t/004template.t,v  <--  004template.t
new revision: 1.29; previous revision: 1.28
done
Checking in t/Support/Templates.pm;
/cvsroot/mozilla/webtools/bugzilla/t/Support/Templates.pm,v  <--  Templates.pm
new revision: 1.13; previous revision: 1.12
done