Closed Bug 899323 Opened 7 years ago Closed 7 years ago

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

Categories

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

Production
defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: dkl, Assigned: dkl)

References

()

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2
modified extensions/Needinfo/Extension.pm
Committed revision 8906.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch 899323_2.patch (obsolete) — Splinter Review
Good point. Here is a patch that better addresses the issue.

dkl
Attachment #783130 - Flags: review?(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.
Attachment #783130 - Flags: review?(glob) → review-
Attached patch 899323_3.patch (obsolete) — Splinter Review
Attachment #783130 - Attachment is obsolete: true
Attachment #783139 - Flags: review?(glob)
Attached patch 899323_4.patchSplinter Review
Just removes the extra @needinfo_overrides check.

dkl
Attachment #783139 - Attachment is obsolete: true
Attachment #783139 - Flags: review?(glob)
Attachment #783159 - Flags: review?(glob)
Attachment #783159 - Flags: review?(glob)
Not neccessary.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → INVALID
Component: Extensions: Needinfo → Extensions
You need to log in before you can comment on or make changes to this bug.