899323 - SENTRY ERROR: Use of uninitialized value in numeric eq (==) at /data/www/bugzilla.mozilla.org/extensions/Needinfo/Extension.pm line 154.

Status: RESOLVED INVALID

(bugzilla.mozilla.org :: Extensions, defect)

Comment 1

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.2
modified extensions/Needinfo/Extension.pm
Committed revision 8906.

Comment 2

[:glob ✱]

that fix doesn't look right to me; the grep will no-op if @needinfo_overrides is empty.
the error indicates that the array isn't empty and contains an undef element.

Comment 3

[David Lawrence [:dkl]]

Attachment: 899323_2.patch

Good point. Here is a patch that better addresses the issue.

dkl

Comment 4

[:glob ✱]

Comment on attachment 783130 [details] [diff] [review]
899323_2.patch

Review of attachment 783130 [details] [diff] [review]:
-----------------------------------------------------------------

i'm not sold that needinfo_overrides is the issue, as the exact same check is made in line 151 without triggering a warning.  it looks like the grep which is part of the condition on line 154 could simply be removed.

as discussed on irc, there's some weirdness around that condition that should be addressed.

::: extensions/Needinfo/Extension.pm
@@ +77,4 @@
>  
>      my @needinfo_overrides;
>      foreach my $key (grep(/^needinfo_override_/, keys %$params)) {
> +        my ($id) = $key =~ /^needinfo_override_(\d+)$/;

as needinfo_override is always in the format of needinfo_override_\d+, i don't think this change impacts the logic.

@@ +81,2 @@
>          # Should always be true if key exists (checkbox) but better to be sure
> +        push(@needinfo_overrides, $id) if detaint_natural($id) && $params->{$key};

$id doesn't need to be detainted, and it's guaranteed to be an int, so detaint_natural is not required.

Comment 5

[David Lawrence [:dkl]]

Attachment: 899323_3.patch

Comment 6

[David Lawrence [:dkl]]

Attachment: 899323_4.patch

Just removes the extra @needinfo_overrides check.

dkl

Comment 7

[David Lawrence [:dkl]]

Not neccessary.