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

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

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.

Comment 1

[Frédéric Buclin]

This is a regression due to bug 238878. The 2.18 branch is not affected.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Comment 3

[Teemu Mannermaa (: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.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 5

[Frédéric Buclin]

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.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

And with this change will it remain possible to "append results to that field in a clean way"?

Comment 7

[Frédéric Buclin]

(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.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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

Comment 9

[Frédéric Buclin]

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

Comment 10

[Frédéric Buclin]

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.

Comment 11

[Frédéric Buclin]

Attachment: 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.

Comment 12

[Dave Miller [:justdave]]

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.

Comment 13

[Frédéric Buclin]

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