Implement code hook mechanism

RESOLVED FIXED in Bugzilla 3.0

Status

()

enhancement
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: zach, Assigned: zach)

Tracking

unspecified
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(5 attachments, 10 obsolete attachments)

26.35 KB, patch
timeless
: review+
Details | Diff | Splinter Review
26.42 KB, patch
Details | Diff | Splinter Review
755 bytes, patch
Details | Diff | Splinter Review
3.90 KB, patch
LpSolit
: review-
Details | Diff | Splinter Review
2.64 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
Assignee

Description

14 years ago
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.
Assignee

Comment 1

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

Comment 2

14 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #186987 - Flags: review?
Assignee

Comment 3

14 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #186988 - Flags: review?
Assignee

Comment 4

14 years ago
Comment on attachment 186988 [details] [diff] [review]
Patch v1

grr double submitted
Attachment #186988 - Attachment is obsolete: true
Assignee

Comment 5

14 years ago
Posted patch v2 (obsolete) — Splinter Review
Attachment #186987 - Attachment is obsolete: true
Attachment #186994 - Flags: review?
Attachment #186987 - Flags: review?
Attachment #186988 - Flags: review?
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.
Assignee

Comment 7

14 years ago
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #186994 - Attachment is obsolete: true
Attachment #187186 - Flags: review?
Assignee

Updated

14 years ago
Attachment #186994 - Flags: review?
Assignee

Comment 8

14 years ago
Comment on attachment 187186 [details] [diff] [review]
v3

r=timeless
Attachment #187186 - Flags: review? → review+
Assignee

Updated

14 years ago
Flags: approval?
(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.
Target Milestone: --- → Bugzilla 2.22
should it be called "extensions" instead of "extension"?  That would be more
consistent with other software packages.
Assignee

Comment 11

14 years ago
Good point. Here's an extra patch snippit to have checksetup create the
directory at install-time.
Attachment #187242 - Flags: review?(justdave)
Assignee

Comment 12

14 years ago
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.
Attachment #187243 - Flags: review?(justdave)

Comment 13

14 years ago
extensions per comment 0 and skins/
Assignee

Comment 14

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

Bah ok. extensions/ it is.
Attachment #187242 - Attachment is obsolete: true
Attachment #187242 - Flags: review?(justdave)
Assignee

Comment 15

14 years ago
Posted patch Combined patch v4 (obsolete) — Splinter Review
(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...
Attachment #187186 - Attachment is obsolete: true
Attachment #187243 - Attachment is obsolete: true
Attachment #187244 - Flags: review?(timeless)
"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".
it would be *really* nice if extensions could add methods to Bugzilla::Bug
Assignee

Comment 18

14 years ago
Well they can as long as there's a hook added to Bugzilla::Bug :)
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.
Assignee

Comment 20

14 years ago
(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. 
> 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.
Assignee

Comment 22

14 years ago
Why couldn't your hook just say "package Bugzilla::Bug;" and add its methods
from there?
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 $@;
    }

Assignee

Comment 24

14 years ago
Posted patch Patch v5 (obsolete) — Splinter Review
Attachment #187244 - Attachment is obsolete: true
Attachment #188801 - Flags: review?(timeless)
Assignee

Comment 25

14 years ago
Posted patch Patch v5.1 (obsolete) — Splinter Review
Sorry. An extra chunk got added in there that was supposted to be gone.
Attachment #188801 - Attachment is obsolete: true
Attachment #188802 - Flags: review?(timeless)
Assignee

Updated

14 years ago
Flags: approval?

Updated

14 years ago
Attachment #187244 - Flags: review?(timeless)

Updated

14 years ago
Attachment #188801 - Flags: review?(timeless)

Comment 26

14 years ago
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 <zach@zachlipton.com>

>@@ -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.
Attachment #188802 - Flags: review?(timeless) → review-
Zach, any update on this patch?

Updated

14 years ago
Attachment #187243 - Flags: review?(justdave)
Assignee

Comment 28

14 years ago
Posted patch Patch v6Splinter Review
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.
Attachment #188802 - Attachment is obsolete: true
Attachment #193314 - Flags: review?
Assignee

Comment 29

14 years ago
I'd appreciate a review if someone is so inclined. 
Assignee

Comment 30

14 years ago
Comment on attachment 193314 [details] [diff] [review]
Patch v6

timeless: do you think you'd be able to review this?
Attachment #193314 - Flags: review? → review?(timeless)

Comment 31

14 years ago
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"
Attachment #193314 - Flags: review?(timeless) → review+

Comment 32

14 years ago
The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
I think we might want to consider looking at Class::Trigger for this, too.
This should be in the approval queue for 2.24.
Flags: approval?
Assignee

Comment 35

13 years ago
Now that we're working on opening the trunk again, I'll update this patch. 
Status: NEW → ASSIGNED
Zach seems to imply the patch needs updating...  does it?
Flags: approval? → approval+

Comment 37

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

Timeless gave some nits in comment #31.

Comment 38

13 years ago
Posted patch Patch v6, updated (obsolete) — Splinter Review
Updated v6 per timeless nits

Comment 39

13 years ago
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 :)
Attachment #213440 - Attachment is obsolete: true

Comment 40

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 41

13 years ago
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

13 years ago
Docs are also broken.

Comment 43

13 years ago
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

13 years ago
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

13 years ago
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

Updated

13 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 46

13 years ago
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 48

13 years ago
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

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

No, Field.pm has already been fixed
Attachment #213456 - Flags: review-

Comment 50

13 years ago
(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

13 years ago
This should be fixed until it's backed out, and I'm still within those 60 minutes.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 52

13 years ago
(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

13 years ago
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

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

Just for the record.. r=LpSolit
Attachment #213458 - Flags: review+

Comment 55

13 years ago
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

13 years ago
(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.

Updated

13 years ago
Blocks: 328848

Comment 57

13 years ago
Yeah, that makes sense. Moved to bug 328848.

Updated

13 years ago
Keywords: relnote
Added to the relnotes currently attached to bug 349423.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.