Bug 525606: Make the template_before_process hook run whenever a template is loaded (including PROCESS and INCLUDE), not just when $template->process is called.

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=dkl, a=mkanat

4 files changed, 135 insertions, 40 deletions

diff --git a/Bugzilla/Hook.pm b/Bugzilla/Hook.pm
index 5093db903..44b5c3c58 100644
--- a/Bugzilla/Hook.pm
+++ b/Bugzilla/Hook.pm
@@ -734,15 +734,25 @@ look at the code for C<create> in L<Bugzilla::Template>.)
 
 =head2 template_before_process
 
+This hook is called any time Bugzilla processes a template file, including
+calls to C<< $template->process >>, C<PROCESS> statements in templates,
+and C<INCLUDE> statements in templates. It is not called when templates
+process a C<BLOCK>, only when they process a file.
+
 This hook allows you to define additional variables that will be available to
-the template being processed. You probably want to restrict your hook
-to operating only if a certain file is being loaded (which is why you
-get a C<file> argument below). Otherwise, modifying the C<vars> argument
-will affect every single template in Bugzilla.
+the template being processed, or to modify the variables that are currently
+in the template. It works exactly as though you inserted code to modify
+template variables at the top of a template.
+
+You probably want to restrict this hook to operating only if a certain 
+file is being processed (which is why you get a C<file> argument
+below). Otherwise, modifying the C<vars> argument will affect every single
+template in Bugzilla.
 
-Note that this is only called on the top-level C<< $template->process >>
-call. It is not called for C<[% PROCESS %]> or C<[% INCLUDE %]> statements 
-in templates.
+B<Note:> This hook is not called if you are already in this hook.
+(That is, it won't call itself recursively.) This prevents infinite
+recursion in situations where this hook needs to process a template
+(such as if this hook throws an error).
 
 Params:
 
@@ -750,28 +760,25 @@ Params:
 
 =item C<vars>
 
-The template vars hashref--these are the values that get passed to the
-template. Adding new keys to this hashref will cause those new values
-to also get passed to the template.
+This is the entire set of variables that the current template can see.
+Technically, this is a L<Template::Stash> object, but you can just
+use it like a hashref if you want.
 
-=item C<file> 
+=item C<file>
 
-The name of the template being processed. This is relative
-to the main template directory for the language (i.e. for
+The name of the template file being processed. This is relative to the
+main template directory for the language (i.e. for
 F<template/en/default/bug/show.html.tmpl>, this variable will contain
 C<bug/show.html.tmpl>).
 
-=item C<template>
+=item C<context>
 
-The L<Bugzilla::Template> object that C<process> was called on.
+A L<Template::Context> object. Usually you will not have to use this, but
+if you need information about the template itself (other than just its
+name), you can get it from here.
 
 =back
 
-B<Note:> This hook is not called if you are already in this hook.
-(That is, it won't call itself recursively.) This prevents infinite
-recursion in situations where this hook needs to process a template
-(such as if this hook throws an error).
-
 =head2 webservice
 
 This hook allows you to add your own modules to the WebService. (See
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index c8e3bc69c..046383094 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -80,25 +80,6 @@ sub _load_constants {
     return \%constants;
 }
 
-# Overload Template::Process in order to add a hook to allow additional
-# variables to be made available by an extension
-sub process {
-    my $self = shift;
-    my ($file, $vars) = @_;
-
-    # This hook can't call itself recursively, because otherwise we
-    # end up with problems when we throw an error inside of extensions
-    # (they end up in infinite recursion, because throwing an error involves
-    # processing a template).
-    if (!Bugzilla::Hook::in('template_before_process')) {
-        Bugzilla::Hook::process('template_before_process', 
-                                { vars => $vars, file => $file, 
-                                  template => $self });
-    }
-
-    return $self->SUPER::process(@_);
-}
-
 # Returns the path to the templates based on the Accept-Language
 # settings of the user and of the available languages
 # If no Accept-Language is present it uses the defined default
@@ -487,7 +468,7 @@ sub create {
         COMPILE_DIR => bz_locations()->{'datadir'} . "/template",
 
         # Initialize templates (f.e. by loading plugins like Hook).
-        PRE_PROCESS => "global/initialize.none.tmpl",
+        PRE_PROCESS => ["global/initialize.none.tmpl"],
 
         ENCODING => Bugzilla->params->{'utf8'} ? 'UTF-8' : undef,
 
@@ -802,6 +783,8 @@ sub create {
         },
     };
 
+    local $Template::Config::CONTEXT = 'Bugzilla::Template::Context';
+
     Bugzilla::Hook::process('template_before_create', { config => $config });
     my $template = $class->new($config) 
         || die("Template creation failed: " . $class->error());
diff --git a/Bugzilla/Template/Context.pm b/Bugzilla/Template/Context.pm
new file mode 100644
index 000000000..7923603e5
--- /dev/null
+++ b/Bugzilla/Template/Context.pm
@@ -0,0 +1,104 @@
+# -*- Mode: perl; indent-tabs-mode: nil -*-
+#
+# The contents of this file are subject to the Mozilla Public
+# License Version 1.1 (the "License"); you may not use this file
+# except in compliance with the License. You may obtain a copy of
+# the License at http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS
+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# rights and limitations under the License.
+#
+# The Original Code is the Bugzilla Bug Tracking System.
+#
+# The Initial Developer of the Original Code is ITA Software.
+# Portions created by the Initial Developer are Copyright (C) 2009 
+# the Initial Developer. All Rights Reserved.
+#
+# Contributor(s):
+#   Max Kanat-Alexander <mkanat@bugzilla.org>
+
+# This exists to implement the template-before_process hook.
+package Bugzilla::Template::Context;
+use strict;
+use base qw(Template::Context);
+
+use Bugzilla::Hook;
+use Scalar::Util qw(blessed);
+
+sub process {
+    my $self = shift;
+    # We don't want to run the template_before_process hook for
+    # template hooks (but we do want it to run if a hook calls
+    # PROCESS inside itself). The problem is that the {component}->{name} of
+    # hooks is unreliable--sometimes it starts with ./ and it's the
+    # full path to the hook template, and sometimes it's just the relative
+    # name (like hook/global/field-descs-end.none.tmpl). Also, calling
+    # template_before_process for hook templates doesn't seem too useful,
+    # because that's already part of the extension and they should be able
+    # to modify their hook if they want (or just modify the variables in the
+    # calling template).
+    if (not delete $self->{bz_in_hook}) {
+        $self->{bz_in_process} = 1;
+    }
+    my $result = $self->SUPER::process(@_);
+    delete $self->{bz_in_process};
+    return $result;
+}
+
+# This method is called by Template-Toolkit exactly once per template or
+# block (look at a compiled template) so this is an ideal place for us to
+# modify the variables before a template or block runs.
+#
+# We don't do it during Context::process because at that time
+# our stash hasn't been set correctly--the parameters we were passed
+# in the PROCESS or INCLUDE directive haven't been set, and if we're
+# in an INCLUDE, the stash is not yet localized during process().
+sub stash {
+    my $self = shift;
+    my $stash = $self->SUPER::stash(@_);
+
+    my $name = $stash->{component}->{name};
+    my $pre_process = $self->config->{PRE_PROCESS};
+
+    # Checking bz_in_process tells us that we were indeed called as part of a
+    # Context::process, and not at some other point. 
+    #
+    # Checking $name makes sure that we're processing a file, and not just a
+    # block, by checking that the name has a period in it. We don't allow
+    # blocks because their names are too unreliable--an extension could have
+    # a block with the same name, or multiple files could have a same-named
+    # block, and then your extension would malfunction.
+    #
+    # We also make sure that we don't run, ever, during the PRE_PROCESS
+    # templates, because if somebody calls Throw*Error globally inside of
+    # template_before_process, that causes an infinite recursion into
+    # the PRE_PROCESS templates (because Bugzilla, while inside 
+    # global/intialize.none.tmpl, loads the template again to create the
+    # template object for Throw*Error).
+    #
+    # Checking Bugzilla::Hook::in prevents infinite recursion on this hook.
+    if ($self->{bz_in_process} and $name =~ /\./
+        and !grep($_ eq $name, @$pre_process)
+        and !Bugzilla::Hook::in('template_before_process'))
+    {
+        Bugzilla::Hook::process("template_before_process",
+                                { vars => $stash, context => $self,
+                                  file => $name });
+    }
+
+    # This prevents other calls to stash() that might somehow happen
+    # later in the file from also triggering the hook.
+    delete $self->{bz_in_process};
+
+    return $stash;
+}
+
+# We need a DESTROY sub for the same reason that Bugzilla::CGI does.
+sub DESTROY {
+    my $self = shift;
+    $self->SUPER::DESTROY(@_);
+};
+
+1;
diff --git a/Bugzilla/Template/Plugin/Hook.pm b/Bugzilla/Template/Plugin/Hook.pm
index 9c292d726..e993060dd 100644
--- a/Bugzilla/Template/Plugin/Hook.pm
+++ b/Bugzilla/Template/Plugin/Hook.pm
@@ -68,6 +68,7 @@ sub process {
 
     # process() accepts an arrayref of templates, so we just pass the whole
     # arrayref.
+    $context->{bz_in_hook} = 1; # See Bugzilla::Template::Context
     return $context->process($cache->{"${lang}__$extension_template"});
 }