318205 - the path to the hook is incorrect when called from inside a block

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

If the template template/en/default/foo/bar/baz.html.tmpl contains a block like:

[% BLOCK bug_display %]

[% Hook.process("external") %]

[% END %]

The hook has to be located at template/en/extension/hook/bug_display/external/ instead of template/en/extension/hook/foo/bar/baz.html.tmpl/external/.

I spent 20 minutes to understand why my hook was ignored! :(

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to allow Hook.process() to work even inside a template block (v1)

When adding in [% Hook.process() %] to a template in order to use an extension
template, it cannot find the extension template correctly if the Hook.process()
in the parent template is inside of a template BLOCK.

For example:

[% BLOCK bug_display %]
  ...

  [% Hook.process('get_text_long') %]

  ...
[% END %]

Bugzilla/Template/Plugin/Hook.pm line 72:

my $template = $self->{_CONTEXT}->stash->{component}->{name};

will see the template name as 'bug_display' and not
'bug/show-multiple.html.tmpl'.

Attaching a patch that seems to fix the problem for me by using
$self->{_CONTEXT}->stash->{component}->{caller} when every $template does not
look like a valid template name. 'caller' is the parent of the template BLOCK.

Dave

Comment 3

[Frédéric Buclin]

Comment on attachment 304748 [details] [diff] [review]
Patch to allow Hook.process() to work even inside a template block (v1)

>+    # If we are in a PROCESS block then get the caller name instead
>+    $template = $template =~ m/(.*)\.(.*)\.tmpl/ ? $template : $caller;

What happens if a PROCESS block is in another PROCESS block? This seems to be pretty common to me, e.g. bug/edit.html.tmpl is PROCESS'ed by many other templates, and it itself PROCESS'es several others, such as footer.html.tmpl.

Comment 4

[David Lawrence [:dkl]]

(In reply to comment #3)
> What happens if a PROCESS block is in another PROCESS block? This seems to be
> pretty common to me, e.g. bug/edit.html.tmpl is PROCESS'ed by many other
> templates, and it itself PROCESS'es several others, such as footer.html.tmpl.

At first thought we would just work out way up the hierarchy til we found a name that looked like a template and then go with that one. But after playing around with this some more today, if you have  PROCESS within another PROCESS, the $self->{_CONTEXT}->stash->{component}->{caller} value is no longer there to access for some reason. There is only {name} and {modtime}. Possibly a template toolkit bug that it only works for one level deep?

Comment 5

[Frédéric Buclin]

-> 4.0 as fixing this bug is going to break hooks which currently workaround this problem. I don't want to regress 3.x now.

Comment 6

[Frédéric Buclin]

(In reply to comment #4)
> if you have  PROCESS within another PROCESS,
> the $self->{_CONTEXT}->stash->{component}->{caller} value is no longer there to
> access for some reason. There is only {name} and {modtime}. Possibly a template
> toolkit bug that it only works for one level deep?

Looks like so. Even $self->{_CONTEXT}->stash->{component}->{callers} (callers with 's') becomes undefined within blocks. It seems that it's working fine if you process a file name though. I wonder if that's a bug in our own Bugzilla::Template::Plugin::Hook module.

I didn't check if we often call a BLOCK from within another BLOCK, but if that's the case, it would be a pain to document the way to write hooks as it would depend whether we have such a dependency or not. That's why I'm still hesitating to approve this patch.

myk, mkanat, any opinion?

Comment 7

[Max Kanat-Alexander]

  We should do whatever is required to make hooks *reliably* pay attention to the file name and not the BLOCK name. If there's a bug in TT that prevents us from doing so, we should report it and get it fixed.

Comment 8

[Elliotte Martin]

One simple way to work around this is to add a new Hook method that accepts a template name parameter and does the same process function or change the existing Hook.process method to accept an optional template name parameter to force the correct name.

Comment 9

[Max Kanat-Alexander]

I'd be in favor of Elliotte's solution, as a workaround for now, and then report this upstream to TT to have them fix it in a better way internally. Does that sound good to everybody?

Comment 10

[David Lawrence [:dkl]]

(In reply to comment #9)
> I'd be in favor of Elliotte's solution, as a workaround for now, and then
> report this upstream to TT to have them fix it in a better way internally. Does
> that sound good to everybody?

Sounds good to me.

Dave

Comment 11

[Frédéric Buclin]

Comment on attachment 304748 [details] [diff] [review]
Patch to allow Hook.process() to work even inside a template block (v1)

Canceling review request based on the last few comments.

Comment 12

[Elliotte Martin]

Attachment: Add optional template name so that Hook.process works correctly inside a template block (v1)

If an optional name parameter is passed it, it will take precedence over the name from the context.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 322403 [details] [diff] [review]
Add optional template name so that Hook.process works correctly inside a template block (v1)

This looks good. You can just do "$template ||=" instead of the ternary operator there, though. (We can fix that on checkin.)

Comment 14

[Frédéric Buclin]

Could you update the POD so that we have a chance to understand how this workaround works?

Comment 15

[Elliotte Martin]

Attachment: Updates POD to document workaround

Comment 16

[Max Kanat-Alexander]

Comment on attachment 322461 [details] [diff] [review]
Updates POD to document workaround

This is good, but it should be above SEE ALSO, not below it. (I know, that's just a tiny thing--I'd fix it myself, but I figure it'd be easier for you to do it than for me to do it, and I'm not in a huge hurry to check it in right at this second.)

Comment 17

[Elliotte Martin]

Attachment: Updates POD to document changes to process method (v2)

Move 'See Also' section to end of POD.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 322604 [details] [diff] [review]
Updates POD to document changes to process method (v2)

  Looks good!

>+[% Hook.process("lastrow", "bug/show-multiple.html.tmpl") %].

  On checkin that should be wrapped in a C<>.

Comment 19

[Max Kanat-Alexander]

I made all the checkin fixes plus another critical POD fix (added a =back) and a small formatting difference in the Perl code.

tip:

Checking in Bugzilla/Template/Plugin/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v  <--  Hook.pm
new revision: 1.11; previous revision: 1.10
done


3.2 branch:

Checking in Bugzilla/Template/Plugin/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v  <--  Hook.pm
new revision: 1.10.2.1; previous revision: 1.10
done

Comment 20

[Max Kanat-Alexander]

Attachment: Checked-In Version

Here's what got checked in.