298341 - Implement code hook mechanism

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Zach Lipton [:zach]]

Now that we have an extension hook mechanism for templates, it would be great if
we had hooks into code as well, so that extensions can drop into existing code
without having to apply patches. 

Code hooks would be executed with the 'do' function so extensions can directly
use variables and subroutines from within the original source file.

One concern: right now it is impossible for more than one extension to use a
template hook. What would people think if we reorganized things a bit so the
tree looks like this? 

bugzilla/
-extensions
--testrunner
---templates
----en/
-----default/etc...
---code
----somefile_with_code_extension.pl
--some_other_extension
--lots_of_extensions

This way it's unlikely that multiple extensions will collide and installation
can be accomplished by just installing into a single directory.

Comment 1

[Zach Lipton [:zach]]

Actually we're smarter then I thought we are. We already handle multiple
extensions per hook. 

Comment 2

[Zach Lipton [:zach]]

Attachment: Patch v1

Here's a first crack at this. Essentially it's two new features: code hooks
(like template hooks, but for code) and extension directories (all extensions
live in bugzilla/extension/extensionname, put the extension there and it's
installed, take it out and it's gone). There's only one code hook at present,
but obviously more can and will be added as needed for various uses and
projects.

Comment 3

[Zach Lipton [:zach]]

Attachment: Patch v1

Here's a first crack at this. Essentially it's two new features: code hooks
(like template hooks, but for code) and extension directories (all extensions
live in bugzilla/extension/extensionname, put the extension there and it's
installed, take it out and it's gone). There's only one code hook at present,
but obviously more can and will be added as needed for various uses and
projects. I made some pretty extensive changes to Customization.xml which
document how the new mechanism works and where everything goes.

Comment 4

[Zach Lipton [:zach]]

Comment on attachment 186988 [details] [diff] [review]
Patch v1

grr double submitted

Comment 5

[Zach Lipton [:zach]]

Attachment: v2

Comment 6

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

Looks pretty good.  A couple points: invoke_hook would probably be better as
Bugzilla::Hook::process (as with templates), where "Hook" is a module that
handles hooks, and you should make sure changing the way hooks work for
templates doesn't break the ability for a custom hook to override an extension hook.

Comment 7

[Zach Lipton [:zach]]

Attachment: v3

Thanks Myk. Here's a new patch addressing those comments. This now lets custom
templates override extension templates and permits hooks in custom/ as well.
See the changes to the docs for how this works. I tested all the permutations
of this and everything that should be able to override something does so. 

As part of checking this in, template/extension should be removed on the cvs
server and bugzilla/extension created.

Comment 8

[Zach Lipton [:zach]]

Comment on attachment 187186 [details] [diff] [review]
v3

r=timeless

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #7)

> As part of checking this in, template/extension should be removed on the cvs
> server and bugzilla/extension created. 

bugzilla/extension, if we're not shipping anything in it, needs to be created by
checksetup.pl.  If it's empty, cvs up -dP will delete it.

Comment 10

[Dave Miller [:justdave]]

should it be called "extensions" instead of "extension"?  That would be more
consistent with other software packages.

Comment 11

[Zach Lipton [:zach]]

Attachment: Additional patch for checksetup to create extension/

Good point. Here's an extra patch snippit to have checksetup create the
directory at install-time.

Comment 12

[Zach Lipton [:zach]]

Attachment: Additional patch for checksetup to create extension/

Good point. Here's an extra patch snippit to have checksetup create the
directory at install-time. 

Calling it extensions/ makes more sense to me. I was going along with our
existing naming convention which has template/ rather than templates/. Let me
know which you would prefer.

Comment 13

[timeless]

extensions per comment 0 and skins/

Comment 14

[Zach Lipton [:zach]]

Comment on attachment 187242 [details] [diff] [review]
Additional patch for checksetup to create extension/

Bah ok. extensions/ it is.

Comment 15

[Zach Lipton [:zach]]

Attachment: Combined patch v4

(ok I promise to press submit once this time)

Here's everything put back together. I renamed the directory to extensions/ and
updated the docs references, checksetup.pl creates the directory, and
everything is happy in the land of Bugzilla...

Comment 16

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

"template" is singular because that directory already existed when I started
templatizing Bugzilla, and I figured reusing it was better than creating another
one.  But in general, these things should be plural, hence "data" and "skins".

Comment 17

[:glob ✱]

it would be *really* nice if extensions could add methods to Bugzilla::Bug

Comment 18

[Zach Lipton [:zach]]

Well they can as long as there's a hook added to Bugzilla::Bug :)

Comment 19

[:glob ✱]

it's strange that the directory structure for code and template hooks is
different.  the one used by templates makes more sense to me, with a slight
modification..

  extensions/testrunner/code/Bugzilla/Bug.pm/hookname.pl


also, as you're using "do" instead of "eval", the code hooks don't have access
to lexivals in the enclosing scope, which severly limits what they can do.

Comment 20

[Zach Lipton [:zach]]

(In reply to comment #19)
> it's strange that the directory structure for code and template hooks is
> different.  the one used by templates makes more sense to me, with a slight
> modification..
>   extensions/testrunner/code/Bugzilla/Bug.pm/hookname.pl
How do you propose that Hook::process() know the path to its caller? Saying
Bugzilla::Hook::process("Bugzilla/Bug.pm", "hookname") is just ugly and a bad
idea if you might ever rename a file later. Also structuring it as I did means
that if you move a hook, extensions don't have to change to accommodate it,
since the hook name can stay the same if needed. This is more of an issue for
code where things get moved pretty often, then in templates which are far more
static. Also, I don't like having directories that look like they are files
(Bug.pm/). 

> also, as you're using "do" instead of "eval", the code hooks don't have access
> to lexivals in the enclosing scope, which severly limits what they can do.
> 
According to perlfunc, "do 'stat.pl'; is just like is just like
    eval `cat stat.pl`;
except that it's more efficient, more concise, [and] keeps track of the current
filename for error messages." Therefore do seems like the better choice for
this. Especially because with eval, any errors in extension code look like they
are in our code, which we certainly don't want. 

Comment 21

[:glob ✱]

> How do you propose that Hook::process() know the path to its caller?

(caller())[1]

> According to perlfunc, "do 'stat.pl'; is just like is just like
>     eval `cat stat.pl`;
> except that it's more efficient, more concise[..]

actually, don't worry about it.  even if you did use eval, it would be called in
the context of Bugzilla::Hook::process() not the caller, so it wouldn't have
access to locals anyhow.

hooks are probably better called "triggers" as they can't hook into any existing
code :(

this also means you can't add methods to Bugzilla::Bug.

Comment 22

[Zach Lipton [:zach]]

Why couldn't your hook just say "package Bugzilla::Bug;" and add its methods
from there?

Comment 23

[:glob ✱]

ah.  indeed you can :)

ok, back to the task at hand.  the current code ignores any errors thrown by the
hooks.  shouldn't it do something like:

    my $filename = $extension.'/code/'.$name.'.pl';
    if (-e $filename) {
        do $filename;
        croak "$filename: $@" if $@;
    }

Comment 24

[Zach Lipton [:zach]]

Attachment: Patch v5

Comment 25

[Zach Lipton [:zach]]

Attachment: Patch v5.1

Sorry. An extra chunk got added in there that was supposted to be gone.

Comment 26

[timeless]

Comment on attachment 188802 [details] [diff] [review]
Patch v5.1

>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>@@ -856,8 +856,14 @@
tabs:
>+# 2005-06-24 Create extension/ if it does not already exist:
>+unless (-d $extensiondir) {
>+	print "Creating extensions directory ($extensiondir) ...\n";
>+	mkdir $extensiondir, 0770;
>+}

>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v
>+our $extensiondir = "$libpath/extensions";

>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v
>+# Templates may also be found in the extension/ tree
extensions?

>@@ -122,13 +122,32 @@
>+    foreach my $extension (@extensions) {
>+    	trick_taint($extension); # since this comes right from the filesystem
tabs:
>+    							 # we have bigger issues if it is insecure

>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v
tabs:
>+# 				  Zach Lipton <[e-mail]>

>@@ -41,9 +48,40 @@
trailing whitespace:
>+        
>+    # also get extension hook files which live in extension/:
extension_s_?


>+# that corresponds with our desired languages:
corresponds _to_ ?

i'm going to stop complaining abouot tabs now, please don't forget to remove
them

>RCS file: /cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v

>+      with installed extensions easier. Furthermore, they make it easy to install 
>+      and remove an extension to Bugzilla, as an extension is nothing more than a 
to/from
>+      simple directory. 
directory or directory structure?


above you definitely meant structure not 'simple directory' given:
>+          <filename>BUGZILLA_ROOT/extensions/template/en/</filename> 
>+          directory. Extension template directories, like the 

>           Installation customizers can also take advantage of hooks when adding
>+          to a Bugzilla template.  To do so, create a directory in
you changed who's doing action (third person to second person implied)
>           <filename>BUGZILLA_ROOT/template/en/custom/hook/</filename>
>+          that corresponds to the hook you wish to use, then place your 
(definitely second person)
>+          customization templates into those directories. For example, 
>+          if you wanted to use the hook "end" in 
>+          <filename>global/useful-links.html.tmpl</filename>, you would 
>+          create the directory <filename>BUGZILLA_ROOT/template/en/custom/hook/
>+          global/useful-links.html.tmpl/end/</filename> and add your customization 
>+          template to this directory.

Comment 27

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

Zach, any update on this patch?

Comment 28

[Zach Lipton [:zach]]

Attachment: Patch v6

For something so simple, v6 seems to be getting rather excessive ;) This patch
addresses timeless's comments. 

Timeless/Myk/whoever else is interested: a review would be great.

Comment 29

[Zach Lipton [:zach]]

I'd appreciate a review if someone is so inclined. 

Comment 30

[Zach Lipton [:zach]]

Comment on attachment 193314 [details] [diff] [review]
Patch v6

timeless: do you think you'd be able to review this?

Comment 31

[timeless]

Comment on attachment 193314 [details] [diff] [review]
Patch v6

should $extensiondir become $extensionsdir?

# also get extension hook files which live in extensions/:

should "which" be "that"

this indentation /looks/ wrong in diff viewer:
sub getLanguages() {
     my $languages = trim(Param('languages'));
     if (not ($languages =~ /,/)) { # only one language
         return $languages;
     }
    my @languages       = Bugzilla::Template::sortAcceptLanguage($languages);

and remove extensions as an extension is nothing more than a 

change "an" to "each"

Comment 32

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 33

[Max Kanat-Alexander]

I think we might want to consider looking at Class::Trigger for this, too.

Comment 34

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

This should be in the approval queue for 2.24.

Comment 35

[Zach Lipton [:zach]]

Now that we're working on opening the trunk again, I'll update this patch. 

Comment 36

[Dave Miller [:justdave]]

Zach seems to imply the patch needs updating...  does it?

Comment 37

[Vlad Dascalu]

(In reply to comment #36)
> Zach seems to imply the patch needs updating...  does it?

Timeless gave some nits in comment #31.

Comment 38

[Vlad Dascalu]

Attachment: Patch v6, updated

Updated v6 per timeless nits

Comment 39

[Vlad Dascalu]

Attachment: Patch v6, checked in

This is the one that will get checked in.

Nit: I don't know about Bugzilla/Hook.pm much, but it seems like a new file and the copyright should not include Netscape. However this makes sense if you're copy-pasting from already existing code.

Nit2: You're getting duplicates in Bugzilla/Config.pm because we're no longer returning (right-away) when we have a single language (because there could be new dirs included by the extensionsdir). So we'll always going to have duplicates when there is only one language. Things could be more efficient, but since it passed already review, I guess efficiency will be left for another bug :)

Comment 40

[Vlad Dascalu]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.473; previous revision: 1.472
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.128; previous revision: 1.127
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.53; previous revision: 1.52
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v
done
Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
initial revision: 1.1
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Template/Plugin/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v  <--  Hook.pm
new revision: 1.2; previous revision: 1.1
done
Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.25; previous revision: 1.24
done

Comment 41

[Vlad Dascalu]

Hmm, the landing broke the tree, trying to fix it since it seems to be minor stuff:

http://tinderbox.mozilla.org/showlog.cgi?log=Bugzilla/1141137964.26982.gz

Basically using ThrowCodeError without a tag, and an invalid POD thingy.

Comment 42

[Vlad Dascalu]

Docs are also broken.

Comment 43

[Vlad Dascalu]

Attachment: Fix docs tree - checked in

Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.26; previous revision: 1.25
done

Comment 44

[Frédéric Buclin]

Comment on attachment 213453 [details] [diff] [review]
Patch v6, checked in

>Index: Bugzilla/Hook.pm

>+            ThrowCodeError("An error occured processing hook \"$name\" in ".
>+                "Bugzilla extension \"$extension\": $@") if $@;



>Index: Bugzilla/Template/Plugin/Hook.pm

>+        ThrowCodeError("Template with invalid file name found in hook call: $template");


Shame on zach and the reviewer for letting these things going through!!

Comment 45

[Frédéric Buclin]

also, next time, run ./runtests.pl before posting a patch....

Failed Test     Stat Wstat Total Fail  Failed  List of Failed
t/002goodperl.t    2   512   342    2   0.58%  310 319
t/011pod.t         2   512   114    2   1.75%  82 91

Comment 46

[Frédéric Buclin]

OK, due to the different places which are broken, a new patch has to be attached (based on the broken one) and should be reviewed. vladd, can you fix it? You're the one who committed it. ;)

Comment 47

[Vlad Dascalu]

Attachment: Fix POD (runtests.pl 11) - checked in

Comment 48

[Vlad Dascalu]

Comment on attachment 213456 [details] [diff] [review]
Fix POD (runtests.pl 11) - checked in

Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Auth/Verify/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm,v  <--  LDAP.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Template/Plugin/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v  <--  Hook.pm
new revision: 1.3; previous revision: 1.2
done

Comment 49

[Frédéric Buclin]

Comment on attachment 213456 [details] [diff] [review]
Fix POD (runtests.pl 11) - checked in

No, Field.pm has already been fixed

Comment 50

[Vlad Dascalu]

(In reply to comment #49)
> (From update of attachment 213456 [details] [diff] [review] [edit])
> No, Field.pm has already been fixed
> 

Yeah, cvs merged them successfully.

Comment 51

[Vlad Dascalu]

This should be fixed until it's backed out, and I'm still within those 60 minutes.

Comment 52

[Frédéric Buclin]

(In reply to comment #51)
> This should be fixed until it's backed out, and I'm still within those 60
> minutes.
> 

Simply fix the two broken ThrowCodeError() and the trunk will stop burning.

Comment 53

[Vlad Dascalu]

Attachment: Fix ThrowCodeError - checked in

Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Template/Plugin/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template/Plugin/Hook.pm,v  <--  Hook.pm
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.64; previous revision: 1.63
done

Comment 54

[Frédéric Buclin]

Comment on attachment 213458 [details] [diff] [review]
Fix ThrowCodeError - checked in

Just for the record.. r=LpSolit

Comment 55

[Vlad Dascalu]

Frederic: thanks!

If the tree gets back to green status, we could/should review the tree fixes and see if we should open new bugs in order to improve them.

One of the fixes required to trim the details of one of the error message - I didn't know exactly how to transmit that into the template as a param - LpSolit, any ideas?

I appologise for breaking the tree -- starting this Monday I use a new machine in order to do the commits, and I haven't been able to talk with its admin yet in order to install all the required Perl modules (in order to run all runtests.pl tests).

Comment 56

[Frédéric Buclin]

(In reply to comment #55)
> One of the fixes required to trim the details of one of the error message - I
> didn't know exactly how to transmit that into the template as a param -
> LpSolit, any ideas?

You could probably pass "$reason => $@" to ThrowCodeError() and then write [% reason FILTER html %] in code-error.html.tmpl.

Comment 57

[Vlad Dascalu]

Yeah, that makes sense. Moved to bug 328848.

Comment 58

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.