394438 - Add a hook for adding template vars to any page (Override Template->process)

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Jesper Birch Jensen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Bugzilla 3.0.1

Is it possible to add a new hook in the show_bug.cgi:
33d32
< use Bugzilla::Hook;
140,141d138
< Bugzilla::Hook::process("show_bug-entrydefaultvars", { vars => $vars });
<

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Max Kanat-Alexander]

Describe why you want it and attach a patch as a unified diff. See the Contributor Guidelines here:

  http://wiki.mozilla.org/Bugzilla:Developers

Comment 2

[Matt Rogers]

Attachment: add new hook show_bug-additionalvars

This patch implements the specified hook, although it gives it a different name.

I want it so that I can access variables from a template hook that I've added to bug/edit.html.tmpl (which I will also be submitting)

Should I update the POD documentation for the additional hook as well?

Comment 3

[Max Kanat-Alexander]

Instead of this, why don't you just hook template->process and add things to the $vars hash based on what template is being processed?

And yes, new hooks always need POD and an example.

Comment 4

[Matt Rogers]

Attachment: add an overload of Template::process with a hook for adding vars

Add an overload to Bugzilla::Template in order to hook Template->process as suggested.

Is this what you were talking about about when you said hook template->process?

If so, and the patch is correct, I'll also submit POD and an example in separate patches.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 389250 [details] [diff] [review]
add an overload of Template::process with a hook for adding vars

>Index: Template.pm
>+    my $self = shift;
>+    my $vars = $_[1];
>+
>+    Bugzilla::Hook::process("template-addvariables", {'vars' => $vars,
>+                                                      'page' => $_[0]});

  Looks great. Just call it template-before_process. Also, call the "page" argument "template".

>+
>+    $_[1] = $vars;

  You don't need to do that, because $vars is a ref to start with, and so will be modified anyway.

>+    return $self->{SUPER}::process(@_);

  Nit: No {} around SUPER.

  Now this needs POD and an example.

Comment 6

[Matt Rogers]

Attachment: add an overload of Template::process with a hook for adding vars - version 2

Second (and hopefully final) version of the actual patch to overload Template::process to add a hook. Should address the earlier comments.

POD and example coming in other attachments.

Comment 7

[Matt Rogers]

Attachment: Add POD to Bugzilla/Hook.pm to describe new hook

Here is the POD for Bugzilla/Hook.pm to describe the new hook.

Comment 8

[Matt Rogers]

Attachment: example for the new template-before_process hook

This is the example code hook for the new template-before_process hook. It should go to extensions/example/code

Comment 9

[Frédéric Buclin]

Write a single patch with all changes in it, not one patch per file.

Comment 10

[Matt Rogers]

Attachment: Full patch

This is the same as attachments 389317 and 389318, just combined into a single patch. The new example file is left separate since i don't have write access to CVS

Comment 11

[Bradley Baetz (:bbaetz)]

This will add overhead for each template, so we need the fixes to stop creating/processing templates multiple times per comment first.

Rather than having a fixed hookname, wouldn't it be better to have a 'before_template_process_$template' hook? Would make the hook .pl file cleaner, I think

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> This will add overhead for each template, so we need the fixes to stop
> Rather than having a fixed hookname, wouldn't it be better to have a
> 'before_template_process_$template' hook? Would make the hook .pl file cleaner,
> I think

  No, there are hundreds of templates, it would be silly to make a hook for each one. It's better to offer a flexible interface for extension authors.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #10)
> The new example file is left separate since i don't have write access to
> CVS

  You don't need to do that. Look up "cvsdo", or just read the patching guide linked from http://wiki.mozilla.org/Bugzilla:Developers

Comment 14

[Matt Rogers]

Attachment: combined patch to add hooks to template->process

Full patch with changes based on last bits of feedback

Comment 15

[Bradley Baetz (:bbaetz)]

(In reply to comment #12)
>   No, there are hundreds of templates, it would be silly to make a hook for
> each one. It's better to offer a flexible interface for extension authors.

Why? Each one is going to be doing something different - separate files is cleaner than a bunch of if statements.

Comment 16

[Max Kanat-Alexander]

(In reply to comment #15)
> Why? Each one is going to be doing something different - separate files is
> cleaner than a bunch of if statements.

  Separate hooks requires us to forever predetermine exactly what files extension authors will want to modify--that just isn't good design, on our part. Cleanliness or lack of cleanliness is the responsibility of the extension author. I can personally think of some very clean ways to implement things for different templates, such as a hash that points to subroutine references.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 389339 [details] [diff] [review]
combined patch to add hooks to template->process

Hey Matt--I'm sorry that I never got around to reviewing this! It's because I was never asked for review--our development process is described here:

http://wiki.mozilla.org/Bugzilla:Developers

Anyhow, this looks fine, and just needs a few adjustments that I can make on checkin (copyright, and some tiny style things, and I might also pass the template object itself to the hook and possibly change some names).

Comment 18

[Max Kanat-Alexander]

Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.111; previous revision: 1.110
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/example/code/template-before_process.pl,v
done
Checking in extensions/example/code/template-before_process.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/code/template-before_process.pl,v  <--  template-before_process.pl
initial revision: 1.1
done

Comment 19

[Max Kanat-Alexander]

Attachment: Checked-In Version

Comment 20

[Max Kanat-Alexander]

Comment on attachment 407417 [details] [diff] [review]
Checked-In Version

Oh, template-before_process is missing in this patch, but it did get checked in properly.