908087 - ordering extension hooks

Status: ASSIGNED

(Bugzilla :: Extensions, enhancement)

Description

[Damien Nozay [:dnozay]]

Attachment: extension-order.diff

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

1. add extensions
2. add more extensions


Actual results:

- some hooks have side effects (e.g. bugmail_recipients modifies its parameters)
- template hooks render in the order they are run
- to change the order you need to change the extension's name / package name which is not desirable.


Expected results:

- order should be able to be set without modifying extension name / package name (e.g. in localconfig)

Comment 1

[Simon Green]

Comment on attachment 793884 [details] [diff] [review]
extension-order.diff

Please set the review flag to one of the suggested reviewers when uploading patches.

Comment 2

[Damien Nozay [:dnozay]]

sorry, was just about to do that.

Comment 3

[Frédéric Buclin]

Isn't this going to conflict with bug 547709?

Comment 4

[Damien Nozay [:dnozay]]

(In reply to Frédéric Buclin from comment #3)
> Isn't this going to conflict with bug 547709?

Technical considerations aside, the last comment on that bug tells me that being able to change the order in which the extensions are processed is a second solution to solve the same problem. My interpretation may be wrong.

Comment 5

[Frédéric Buclin]

Comment on attachment 793884 [details] [diff] [review]
extension-order.diff

>=== modified file 'Bugzilla/Install/Util.pm'

>+sub default_extension_paths {
>+    my $dir = bz_locations()->{'extensionsdir'};
>+    my @extension_items = glob("$dir/*");
>+    my @paths;
>+    foreach my $item (@extension_items) {
>+        my $basename = basename($item);
>+        # Skip CVS directories and any hidden files/dirs.
>+        next if ($basename eq 'CVS' or $basename =~ /^\./);
>+        if (-d $item) {
>+            # we're not checking if disabled here
>+            # that will be done when we actually need them
>+            push(@paths, $item);
>+        }
>+        elsif ($item =~ /\.pm$/i) {
>+            push(@paths, $item);
>+        }
>+    }
>+    return \@paths;
>+}

You are duplicating most code from _extension_paths(), which is bad. You really don't need a separate method for that.


> sub _extension_paths {
>+    require Bugzilla;

If you need to require Bugzilla.pm from another Bugzilla::* module, you are in trouble and you area doing something wrong. Bugzilla.pm must be loaded by the .pl or .cgi script itself.


>+    my @extension_paths = $lc_hash->{'extension_paths'};
>+    if (!scalar @extension_paths) {
>+        my @extension_items = @{default_extension_paths()};
>+    }

I don't like this. If extension_paths is set, then it totally overrides enabled extensions (those with no 'disabled' file). This is going to bring confusion as one may not understand why a new extension is not active despite 'disabled' has been removed. IMO, this parameter should only say in which order to load extensions, and have some default behavior for other active extensions (either executed first or last). You could even have two parameters, one for extensions which must be loaded first, and another parameter for extensions which must loaded last. I'm not saying you must do it, I only say this could be an alternative.

Comment 6

[Damien Nozay [:dnozay]]

> You are duplicating most code from _extension_paths(), which is bad. You
> really don't need a separate method for that.

_extension_paths checks for "$item/disabled".
my Perl is very rusty and I don't know how to factor this piece out.
suggestions welcome.

> > sub _extension_paths {
> >+    require Bugzilla;
> 
> If you need to require Bugzilla.pm from another Bugzilla::* module, you are
> in trouble and you area doing something wrong. Bugzilla.pm must be loaded by
> the .pl or .cgi script itself.
> 

I am open to suggestions on how to fix that.
- Bugzilla/Install/Localconfig.pm does "use Bugzilla::Install::Util"
- I can't do a "use Bugzilla::Install::Localconfig" in  Bugzilla/Install/Util.pm, that's an import loop.
- I can't do a "use Bugzilla;" and use Bugzilla->localconfig, that's an import loop as well.

Bugzilla->localconfig should be available by the time we are calling _extension_paths().

> 
> >+    my @extension_paths = $lc_hash->{'extension_paths'};
> >+    if (!scalar @extension_paths) {
> >+        my @extension_items = @{default_extension_paths()};
> >+    }
> 
> I don't like this. If extension_paths is set, then it totally overrides
> enabled extensions (those with no 'disabled' file). This is going to bring
> confusion as one may not understand why a new extension is not active
> despite 'disabled' has been removed. IMO, this parameter should only say in
> which order to load extensions, and have some default behavior for other
> active extensions (either executed first or last). You could even have two
> parameters, one for extensions which must be loaded first, and another
> parameter for extensions which must loaded last. I'm not saying you must do
> it, I only say this could be an alternative.

Overriding the enabled extensions is exactly the point.
But, if for some obscure reason, maintainers prefer a magic glob() to do the job, then they can set extension_paths = () and it will keep the default magic.
This piece of info can go in the documentation for this field.

'disabled' serves only one purpose : to cope with the fact that there is a glob() magic happening. If there is an explicit ordered list, then there is no need for a 'disabled' file; if one wants to change the extensions enabled then one can change the list in localconfig. I didn't change the logic in _extension_paths() checking for disabled, because I don't want to freak out current maintainers, and even if one decides to go with the explicit list of extensions, they can disable the extension with the disabled file.

I do like the first/last idea, but I feel that it is unnecessary, the order of the list *is* the order.


Compare this feature with django's  INSTALLED_APPS setting:
https://docs.djangoproject.com/en/dev/ref/settings/#std:setting-INSTALLED_APPS

Comment 7

[Frédéric Buclin]

(In reply to Damien Nozay [:dnozay] from comment #6)
> Overriding the enabled extensions is exactly the point.

You say overriding is the point, which means ignoring 'disabled', but on the other hand, you say you still look at 'disabled'. Contradiction, maybe? ;)

Comment 8

[Damien Nozay [:dnozay]]

if the enabled extensions are always listed as (A, B, C) and for my use case I need (C, A, B) then yes, overriding is the proper term; 'order' and 'disabled' are orthogonal.

After/if this change goes in, then 'disabled' can become deprecated subsequently; where "item is listed" == "item is enabled" <=> "item is not listed" == "item is disabled". The assumption is that if the item is not listed, then it is effectively disabled and we won't even check for the "disabled" file.

Comment 9

[Damien Nozay [:dnozay]]

Attachment: extension-order-v2.diff

reworked based on suggestions from Frédéric.

Comment 10

[Simon Green]

Comment on attachment 794326 [details] [diff] [review]
extension-order-v2.diff

#   Failed test 'Bugzilla/Install/Util.pm POD coverage is 95%. Undocumented methods: default_extension_paths'
#   at t/011pod.t line 101.
# Looks like you failed 1 test of 340.

Plus run runtests.pl before submitting a patch. If it does not pass, I will not consider it.

Comment 11

[Damien Nozay [:dnozay]]

Attachment: extension-order-v3.diff

fix POD coverage

Comment 12

[Simon Green]

Comment on attachment 794764 [details] [diff] [review]
extension-order-v3.diff

When running checksetup.pl, it adds all extensions in the @extension_paths array. It should create a blank array, so that the existing functionality is maintained. There are some installations that have a lot of extensions (brc and bmo especially) and having to add them for each release could be troublesome.

If not that, it should at least not added the disabled extension.

Additionally, you'll need to modify t/Support/Files.pm so that if the array is specified, the correct extensions will be loaded.

Comment 13

[Damien Nozay [:dnozay]]

(In reply to Simon Green from comment #12)
> Comment on attachment 794764 [details] [diff] [review]
> extension-order-v3.diff
> 
> When running checksetup.pl, it adds all extensions in the @extension_paths
> array. It should create a blank array, so that the existing functionality is
> maintained.

Existing functionality is maintained.
If an extension is disabled, there will be a 'disabled' file; regardless of whether it is listed in @extension_paths.

> There are some installations that have a lot of extensions (brc
> and bmo especially) and having to add them for each release could be
> troublesome.

For installations that have a lot of extensions, adding them for each release could be troublesome, but it is handled as well:

"
When absent, checksetup.pl will (re-)populate this variable by doing a glob() on your extensions directory.
"

> If not that, it should at least not added the disabled extension.
I have explained the rationale in comment #8.
Doing what you suggest means the existing functionality (disabling an extension by creating a 'disabled' file, enabling it by removing the 'disabled' file) will stop working. This should be a separate change.

If you still think this is the preferred behavior, I can make the change.

> Additionally, you'll need to modify t/Support/Files.pm so that if the array
> is specified, the correct extensions will be loaded.
ok, I will work on that.

Comment 14

[Damien Nozay [:dnozay]]

Attachment: v4

Comment 15

[Damien Nozay [:dnozay]]

Comment on attachment 798242 [details] [diff] [review]
v4

https://github.com/dnozay/bugzilla/commit/487f8918cee17c41902547f3b183e8c7db89ab6b
https://travis-ci.org/dnozay/bugzilla/builds/10848467

Comment 16

[Peter Retzer (:pretzer)]

Comment on attachment 798242 [details] [diff] [review]
v4

Setting review to a suggested reviewer, so this doesn't get lost.

Comment 17

[Simon Green]

We definitely plan to change the extension code, including the order in which they run. The change proposed in this patch isn't the way to do it though. Once the blocker is fixed, this bug can be closed.

  -- simon