712108 - New hook for filtering active custom fields in Bugzilla.pm

Status: RESOLVED WONTFIX

(Bugzilla :: Extensions, enhancement)

Description

[David Lawrence [:dkl]]

Need a hook in Bugzilla->active_custom_fields to be able to remove/add fields to the list based on the needs of the extension. This helps to keep similar filtering out of the templates themselves where Bugzilla.active_custom_fields is used in many places.

Propose to implement it as Bugzilla::Hook::process('active_custom_fields', { fields => \$fields });

Patch coming.

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add active_custom_fields hook to Bugzilla.pm (v1)

Comment 2

[Frédéric Buclin]

I think this is wrong. Altering the list of active custom fields would mean that the name of the method is inaccurate. Also, once the list is cached with some given additional criteria, all subsequent calls to this method with different criteria (or no criteria at all besides the default ones) will return the wrong results. Could you elaborate a bit more what you try to achieve exactly? Having an extension which alters this method is prone to break many places.

Comment 3

[David Lawrence [:dkl]]

To give context on why we needed this for BMO, currently we have a customizations in the templates to filter out certain custom fields based on product and sometimes component.

Template customization:

[% FOREACH field = Bugzilla.active_custom_fields %]
  [% NEXT IF NOT user.id AND field.value == "---" %]
  [% NEXT IF cf_hidden_in_product(field.name, bug.product, bug.component, 1) %]

This will send all of the fields to the template regardless whether they will be displayed or not and then loop over all to filter them out. 

With the new extension hook in Bugzilla->active_custom_fields the field list is pre-filtered before it gets to the template alleviating the function call in the template itself for each field.

So now we would just have:

[% FOREACH field = Bugzilla.active_custom_fields(product=>bug.product,type=>1) %]

The same lines in edit.html.tmpl would have the component also being passed in.

[% FOREACH field = Bugzilla.active_custom_fields(product=>bug.product,component=>bug.component,type=>1) %]

type=>1 has special meaning to our hook code in extensions/BMO/Extension.pm and can be 1 or 2.

We have some additional code in Bugzilla->active_custom_fields that uses the params passed in to generate an unique cache identifier so that you don't get back incorrect results.

sub active_custom_fields {
    my ($class, $params) = @_;
    my $cache_id = 'active_custom_fields';
    if ($params) {
        $cache_id .= ($params->{product} ? '_p' . $params->{product}->id : '') .
                     ($params->{component} ? '_c' . $params->{component}->id : '') .
                     ($params->{type} ? '_t' . $params->{type} : '');
    }
    if (!exists $class->request_cache->{$cache_id}) {
        my $fields = Bugzilla::Field->match({ custom => 1, obsolete => 0});
        Bugzilla::Hook::process('active_custom_fields',
                                { fields => \$fields, params => $params });
        $class->request_cache->{$cache_id} = $fields;
    }
    return @{$class->request_cache->{$cache_id}};
}

This way you only get the fields back for the exact settings in params and not an incorrect list of fields if active_custom_fields is called without params for example.

But I felt this was too specialized to BMO and just created a more generic hook setup for this patch. We only expect for now to just need product, component, and type to be passed in with the params. I am fine with this being a BMO only change but I thought it could be useful for those who needed it for other reasons.

dkl

Comment 4

[Max Kanat-Alexander]

Okay, I understand your use case. Thanks for the patch! However, this is exactly the sort of thing that 4.2's Bugzilla->fields is for. If you're going to hook something, that would be the thing to hook here. There's even a chance that it already does what you need (although I'm not totally certain).

Also, instead of your cf_hidden_in_product function, you should probably use the existing methods on the Bugzilla::Field objects so that it can re-use whatever caching might be available.