User::match_field() now leaves fields undefined instead of as empty strings when no value is passed

RESOLVED FIXED in Bugzilla 2.20

Status

()

--
critical
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

({regression})

2.20
Bugzilla 2.20
regression
Bug Flags:
approval +
blocking2.22 +
approval2.20 +
blocking2.20.1 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

13 years ago
2.18.4, User.pm, match_field():

        $vars->{'form'}->{$field}  = '';


2.20 and above, same function:

        # When we add back in values later, it matters that we delete
        # the param here, and not set it to '', so that we will add
        # things to an empty list, and not to a list containing one
        # empty string
        $cgi->delete($field);


The consequence of this change is that if a field passed to User::match_field is empty, 2.20 and above makes it undefined, while 2.18.4 leave it as an empty string. More precisely, a test later in the caller (e.g. process_bug.cgi when checking the QA contact) of the form "if defined($cgi->param('qa_contact'))" returns false instead of true when no QA contact is given. Among other, this prevents us from deleting the QA contact. Maybe there are other (hidden) regressions.

The right fix is to set the field back to '' if the field is empty.
(Assignee)

Comment 1

13 years ago
This is a regression due to bug 238878. The 2.18 branch is not affected.
Flags: blocking2.22?
Flags: blocking2.20.1?
(Assignee)

Comment 2

13 years ago
Created attachment 202078 [details] [diff] [review]
patch, v1
Assignee: wicked → LpSolit
Status: NEW → ASSIGNED
Attachment #202078 - Flags: review?(wicked)
Comment on attachment 202078 [details] [diff] [review]
patch, v1

Tested on both 2.20 branch and trunk and fixes the problem reported in comment #0.
Attachment #202078 - Flags: review?(wicked) → review+
Flags: approval?
Flags: approval2.20?
Ok, so there are regressions, but the comment clearly shows that we made the regression-causing change for a reason.  If we change it back to the old behavior, aren't we going to regress whatever the original change fixed?  It's not clear from the patch or the comments in this bug that the patch fixes the regression without introducing one.
(Assignee)

Comment 5

13 years ago
Previously, the field was reset to '' when there was no match. When we removed %::FORM from the code in 2.19, wicked had to delete the field first in order to append results to that field in a clean way; but we forgot ("we" because I reviewed his patch) to fall back to '' when there was no match for that field, meaning that the field was left undefined, instead of defined but empty. So when you test "if (defined foo)", it returns false instead of true. This regression clearly appears in process_bug.cgi.
And with this change will it remain possible to "append results to that field in a clean way"?
(Assignee)

Comment 7

13 years ago
(In reply to comment #6)
> And with this change will it remain possible to "append results to that field
> in a clean way"?

Results have already been appended (in a clean way). We fall back to an empty string just before returning from User::match_field(). If the function found some matches, we let the field as is.
Great, thanks.  That helps me understand the fix.  This seems important, since not being able to delete the QA contact is a serious regression.  a=myk for 2.22 and 2.20
Flags: blocking2.22?
Flags: blocking2.22+
Flags: blocking2.20.1?
Flags: blocking2.20.1+
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
(Assignee)

Comment 9

13 years ago
tip:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.94; previous revision: 1.93
done

2.20:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.61.2.11; previous revision: 1.61.2.10
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

13 years ago
I'm not convince by my fix. If the field is of type 'single', it should return an empty string. If the field is of type 'multi', it should return an empty array.

I'll investigate a bit and I may back this patch out if required.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

13 years ago
Created attachment 203819 [details] [diff] [review]
patch, v2

only fall back to "" when the field is of type 'single'. Else returning an empty array is identical to let this field undefined.
Attachment #202078 - Attachment is obsolete: true
Attachment #203819 - Flags: review?(justdave)
(Assignee)

Updated

13 years ago
Attachment #203819 - Flags: review?(wicked)
Comment on attachment 203819 [details] [diff] [review]
patch, v2

this matches what we came up with after a massive discussion on IRC the other day.  Looks good to me.
Attachment #203819 - Flags: review?(wicked)
Attachment #203819 - Flags: review?(justdave)
Attachment #203819 - Flags: review+
(Assignee)

Comment 13

13 years ago
tip:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.95; previous revision: 1.94
done

2.20:

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.61.2.13; previous revision: 1.61.2.12
done
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.