The current way of having extensions be a bunch of code that is loaded with 'do' is suboptimal. We have to stat a bunch of files all the time, it can't be preloaded/precompiled with mod_perl, and its ugly. Instead, there should just be Bugzilla::Extension::<foo>, which is loaded at startup, inherit from Bugzilla::Extension, and has subs that called when appropriate, for the hooks. Optionally, B::E can backwards compat the existing setup, but given the low installbase out there that may not be needed. For bonus style, have checksetup do some magic that writes out to localconfig @enabled_extensions to avoid stat on each run. Also see bug 430013 for @INC changes.

Actually, with Moose, we could do even better things than this, I think. But this would be a good starting point. I'm not sure I want the namespace to be Bugzilla::Extension, or what, but it'll be something like that. I would ideally like to avoid having to run checksetup to enable/disable extensions, as I would at some point like to have a web interface for enabling/disabling them.

Then you'd have to play with template paths and cache stuff from the web interface, which is not necessarily a good thing....

Okay, I think I've got it! We can have people continue to implement extensions as they are now, but compile them on demand as a module called extension::<name>::code::<hook name> with a single method called "run"--a little bit like a mini-mod_perl solution. I don't want to force people to put "package blah::blah::blah::" into their extension code, this is why I'm thinking about it this way. However, we could also require that. Honestly, I'm not that concerned yet about this aspect of things--I've added some pretty intense "do" hooks to bmo and I haven't seen a serious performance regression from it, I don't think.

Or, we could just make people implement all their extensions as perl modules with the package name right and everything. Sigh, sometimes I wish we weren't doing this in Perl.

Okay, here's what I think we're going to do: Extension code becomes .pm files, with packages like: extensions::testopia::code::Bug And then the individual hooks are subroutines. Each subroutine is passed an instance of extensions::testopia::code::Bug--which is the same every time any part of that particular package is invoked. That is, we instantiate the object only once, no matter how many times we call any part of that hook. The default new() just blesses an empty hashref as the class. If any extension writers have a problem with this, please speak now or forever hold your peace. :-) I'm thinking of switching to this system wholesale, and not providing backwards-compatibility. It should be very easy to move to--just copying the .pl code into the right places in a .pm file.

How does this work with extending Bug.pm? I mean, if Testopia needs to add methods to Bug.pm or override existing methods for a particular bug, I don't see how having only a singlton instance will help. Maybe I am not getting the gist of it?

This has nothing to do with extending Bugzilla modules. This is a replacement of the system in use for the current extensions/<name>/code/ directory. Instead of the current .pl files we would have these .pm files.

Ah, that makes better sense. So you are proposing that for every Bugzilla module (Bug.pm or Product.pm for example) we have a hook module in Extensions::<Extention>::Code::<Module> and then set the actual guts as subroutines instead of separate .pl scripts for each?

(In reply to comment #9) > Ah, that makes better sense. > So you are proposing that for every Bugzilla module (Bug.pm or Product.pm for > example) we have a hook module in Extensions::<Extention>::Code::<Module> and > then set the actual guts as subroutines instead of separate .pl scripts for > each? Pretty much. It wouldn't necessarily map one-to-one for Bugzilla modules. The module name would the word before the - in the current hooks, for now.

I've thought about this more, and I think that actually, instead of having a bunch of modules, we should just have two: Hooks.pm - A single module containing subroutines for each hook. I think that this will require renaming hooks so that the start with a "blah_" instead of "blah-", since I don't think "-" is a valid character in a subroutine name in Perl. It will have the package name extensions::<blah>::Hooks, and instead of returning "1", it will return <blah>, so that we know what package name to use. Also, in checksetup.pl, we can then warn people that they have their hooks in the wrong directories, during the final checks. Requirements.pm - A single module containing REQUIRED_MODULES and OPTIONAL_MODULES. This has to be separate, since we can't load Hooks.pm if the REQUIRED_MODULES aren't installed. I decided to go with this scheme after writing several extensions myself. Most extensions are actually very simple, and forcing people to separate out the code into a lot of separate files just gets annoying, I think. If people are concerned about load-time performance under mod_cgi, what they can do is separate out the actual code for their hooks into different packages of their own, and "require" those modules inside of their hook subroutines.

Almost done, just have to work out a few bugs.

Okay, here's the new extension system, it's a pretty big patch. :-) I plan to add a few more features to it, but this is the base and I want to check this in before anything else. I'll be sending an email to the developers list with a rough outline of how this works (and what all the changes are), but the docs of the system are also in Bugzilla::Extension, which should be pretty clear.

There was some bitrot, I had to merge up to HEAD.

This checkin involved moving a LOT of code, and thus in CVS, removing and adding a LOT of files. Instead of the traditional checkin message, I'm just going to list what was changed, because I think the traditional message might end up being too long of a comment for Bugzilla: CVS: Modified Files: CVS: Bugzilla.pm checksetup.pl colchange.cgi editparams.cgi CVS: editproducts.cgi enter_bug.cgi mod_perl.pl page.cgi CVS: post_bug.cgi sanitycheck.cgi Bugzilla/Attachment.pm CVS: Bugzilla/Bug.pm Bugzilla/Config.pm Bugzilla/Error.pm CVS: Bugzilla/Flag.pm Bugzilla/Hook.pm Bugzilla/Mailer.pm CVS: Bugzilla/Object.pm Bugzilla/Search.pm Bugzilla/Template.pm CVS: Bugzilla/Auth/Login/Stack.pm Bugzilla/Auth/Verify/Stack.pm CVS: Bugzilla/DB/Schema.pm Bugzilla/Install/DB.pm CVS: Bugzilla/Install/Requirements.pm Bugzilla/Install/Util.pm CVS: template/en/default/global/code-error.html.tmpl CVS: template/en/default/setup/strings.txt.pl CVS: Added Files: CVS: Bugzilla/Extension.pm extensions/BmpConvert/Config.pm CVS: extensions/BmpConvert/Extension.pm CVS: extensions/BmpConvert/disabled extensions/Example/Config.pm CVS: extensions/Example/Extension.pm extensions/Example/disabled CVS: extensions/Example/lib/AuthLogin.pm CVS: extensions/Example/lib/AuthVerify.pm CVS: extensions/Example/lib/ConfigExample.pm CVS: extensions/Example/lib/WSExample.pm CVS: extensions/Example/template/en/default/admin/params/example.html.tmpl CVS: extensions/Example/template/en/default/pages/example.html.tmpl CVS: extensions/Example/template/en/default/setup/strings.txt.pl CVS: extensions/Example/template/en/hook/admin/sanitycheck/messages-statuses.html.tmpl CVS: extensions/Example/template/en/hook/global/user-error-errors.html.tmpl CVS: Removed Files: CVS: extensions/bmp_convert/disabled extensions/bmp_convert/info.pl CVS: extensions/bmp_convert/code/attachment-process_data.pl CVS: extensions/example/disabled extensions/example/info.pl CVS: extensions/example/code/attachment-process_data.pl CVS: extensions/example/code/auth-login_methods.pl CVS: extensions/example/code/auth-verify_methods.pl CVS: extensions/example/code/bug-columns.pl CVS: extensions/example/code/bug-end_of_create.pl CVS: extensions/example/code/bug-end_of_create_validators.pl CVS: extensions/example/code/bug-end_of_update.pl CVS: extensions/example/code/bug-fields.pl CVS: extensions/example/code/bug-format_comment.pl CVS: extensions/example/code/buglist-columns.pl CVS: extensions/example/code/colchange-columns.pl CVS: extensions/example/code/config-add_panels.pl CVS: extensions/example/code/config-modify_panels.pl CVS: extensions/example/code/config.pl CVS: extensions/example/code/flag-end_of_update.pl CVS: extensions/example/code/install-before_final_checks.pl CVS: extensions/example/code/mailer-before_send.pl CVS: extensions/example/code/object-before_create.pl CVS: extensions/example/code/object-before_set.pl CVS: extensions/example/code/object-end_of_create_validators.pl CVS: extensions/example/code/object-end_of_set_all.pl CVS: extensions/example/code/object-end_of_update.pl CVS: extensions/example/code/page-before_template.pl CVS: extensions/example/code/product-confirm_delete.pl CVS: extensions/example/code/sanitycheck-check.pl CVS: extensions/example/code/sanitycheck-repair.pl CVS: extensions/example/code/template-before_create.pl CVS: extensions/example/code/template-before_process.pl CVS: extensions/example/code/webservice-error_codes.pl CVS: extensions/example/code/webservice.pl CVS: extensions/example/lib/AuthLogin.pm CVS: extensions/example/lib/AuthVerify.pm CVS: extensions/example/lib/ConfigExample.pm CVS: extensions/example/lib/WSExample.pm CVS: extensions/example/lib/Bugzilla/ExampleHook.pm CVS: extensions/example/template/en/default/admin/params/example.html.tmpl CVS: extensions/example/template/en/default/pages/example.html.tmpl CVS: extensions/example/template/en/hook/admin/sanitycheck/messages-statuses.html.tmpl CVS: extensions/example/template/en/hook/global/user-error-errors.html.tmpl

Added to the release notes in bug 547466.