161203 - Bug changes with intermediate pages munges fields with multiple values (e.g., CC)

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Randall M! Gee]

Sometimes, when changing a bug, Bugzilla has to display an intermidiate
page.  Examples include specifying the component/version/milestone when a
product is changed, the user logging in, resolving "mid-air collisions",
and confirming duplicates.

In that intermediate page, Bugzilla needs to save the changes the user
wanted to make.  However, for fields with multiple values, e.g., the CC
lists, these values are munged.  All the values are concatenated together
into one value.

To see this in action, take a bug with CCs.  Change the product
and remove a couple of CCs at the same time, so you get the "verify
component/version/milestone" page.  Once you specify these values, you
get an error, because "[e-mail][e-mail]"
is not a registered Bugzilla account.

Note that this was noticed in bug 101056.  But it was only noticed
and fixed for the case where this user has to log in before committing
his change.  It still exists in the other cases.

I discovered this bug in 2.14, but it still exists in 2.16.

I have a fix for this, and I will be submitting them.

Comment 1

[Randall M! Gee]

Attachment: Patch to write %::MFORM values, if the field has multiple values

The problem is that the intermediate pages write out the values in %::FORM
as hidden fields.  Unfortunately, %::FORM just concatenates multiple values.
To get all the values, you need to look at %::MFORM.  This patch will write
the values of %::MFORM{"field"} if "field" contains multiple values.

Comment 2

[Randall M! Gee]

Attachment: Patch to write %::MFORM values, instead of %::FORM values

Now, you may notice that the preceding patch checks $::MFORM{"field"}, to
see if it has multiple values, but writes $::FORM{"field"} if it doesn't.

That's a bit weird, because if it doesn't, $::MFORM{"field"} should still
contain the same thing as $::FORM{"field"}, just in list form.	So this patch
always uses %::MFORM, and completely ignores %::FORM.

I'm submitting both patches, because the fix for the case where the user
has to log-in uses both %::FORM and %::MFORM, so I felt that one patch
should use that method.  Also, it's possible that some scripts re-write
%::FORM values, so checking both values may just being cautious.  (But,
if you're re-writing %::FORM, wouldn't the new values be what you want to
save anyway?  It seems excessively cautious.)

Comment 3

[Randall M! Gee]

Attachment: Patch to write %::MFORM values, instead of %::FORM values (corrected)

Bleah.	I reversed the second patch, and left some debugging in.  I've
corrected it now.

Comment 4

[Randall M! Gee]

Attachment: Patch to write %::MFORM values, instead of %::FORM values (really corrected)

What the...?!  (splutter)  OK, this time I'm really correcting the second
patch.

Comment 5

[Dave Miller [:justdave]]

OK, to what extent are things being rewritten in %::FORM?  I'm sure the correct
approach is to use %::MFORM if there's more than one value and %::FORM if not,
since it's quite possible the scripts might be doing some internal rewriting of
things.

Apparently things have changed enough since 2.16 that this won't apply to the
trunk.  We'll need it for both anyway, this is worth backporting to the 2.16
branch I think. (well, the existing patch applies cleanly there anyway, we'll
have to port it to the trunk)

Comment 6

[J. Paul Reed [:preed]]

Comment on attachment 95136 [details] [diff] [review]
Patch to write %::MFORM values, instead of %::FORM values (really corrected)


I'm needs-working this for two reasons, but I'll be happy to reconsider when I
get an answer to my questions.

>diff -ur bz216box/bugzilla-2.16/CGI.pl CGI.pl
>--- bz216box/bugzilla-2.16/CGI.pl	Tue Jul  9 23:27:15 2002
>+++ CGI.pl	Mon Aug  5 11:08:19 2002
>@@ -824,15 +824,10 @@
>         # Add all the form fields into the form as hidden fields
>         # (except for Bugzilla_login and Bugzilla_password which we
>         # already added as text fields above).
>-        foreach my $i ( grep( $_ !~ /^Bugzilla_/ , keys %::FORM ) ) {
>-          if (defined $::MFORM{$i} && scalar(@{$::MFORM{$i}}) > 1) {
>-            # This field has multiple values; add each one separately.
>-            foreach my $val (@{$::MFORM{$i}}) {
>-              print qq|<input type="hidden" name="$i" value="@{[value_quote($val)]}">\n|;
>-            }
>-          } else {
>-            # This field has a single value; add it.
>-            print qq|<input type="hidden" name="$i" value="@{[value_quote($::FORM{$i})]}">\n|;
>+        foreach my $i ( grep( $_ !~ /^Bugzilla_/ , keys %::MFORM ) ) {
>+          # This field has multiple values; add each one separately.
>+          foreach my $val (@{$::MFORM{$i}}) {
>+            print qq|<input type="hidden" name="$i" value="@{[value_quote($val)]}">\n|;
>           }
>         }

Is there a reason you're changing this code in CGI.pl?

The way I'm reading it, your code may be slightly faster, but it doesn't fix
any bug or change the logic of the code at all.

Is that correct?

Also, this patch doesn't apply cleanly to the trunk; this will probably get
applied to a 2.16.1 when we get ready to ship that, but it will also need to
apply cleanly to the trunk.

You'll probably have to have 2 versions of this patch: a 2.16 branch version
and a trunk version.

Comment 7

[Bradley Baetz (:bbaetz)]

Bleh. It looks like bug 101056 is regressed on the trunk, too. (Which is why
this doesn't apply - bug 158660 templatised the login stuff)

Anyway, this looks fine, I guess. The first comment says that this happens in
2.14 too, though - do we want this for 2.16?

Comment 8

[J. Paul Reed [:preed]]

... or 2.14.4, for that matter...

Comment 9

[Randall M! Gee]

Repsonding to comment 6...

The change to the code in CGI.pl does change the logic.  (Whether or not
it fixes any bug is another story.)

In attachment 95136 [details] [diff] [review], I'm using the principle that the %::MFORM values
are the ones that should be preserved in the intermediate page, not the
%::FORM values.  So I changed CGI.pl to save only the %::MFORM values
when putting up the login page.

In attachment 94103 [details] [diff] [review], I use the "save single-valued %::FORM values, and
multiply-valued %::MFORM values", which is what was already in CGI.pl,
so there isn't any change.

I'll take a look at the trunk to see what needs to be done to get this
patch working for it.  I'll make sure bug 101056 is re-fixed in the
trunk, too.

I do have a patch for this against 2.14.1-3 as well, if you want it for
2.14.4.

Comment 10

[Bradley Baetz (:bbaetz)]

Yeah, the patch looks right, because %FORM values are in %MFORM too. The form
code sucks...

Comment 11

[Randall M! Gee]

Attachment: Patch against trunk, write %::MFORM only if multi-valued

This patch goes against the trunk, or whatever it is I got with a
cvs update -A -dP.  It uses the logic of "write %::FORM, unless
%::MFORM has multiple values, in which case, write %::MFORM."

Comment 12

[Randall M! Gee]

Attachment: Patch against trunk, write %::MFORM always

This patch also goes against the trunk, but it always writes
the %::MFORM values and ignores the %::FORM values.

Comment 13

[Randall M! Gee]

Attachment: Patch against 2.14.3

This patch goes against 2.14.3 (and should fit with the other 2.14 versions.)

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 95641 [details] [diff] [review]
Patch against trunk, write %::MFORM always

This won't work, because 'empty' values are only put into $FORM, not $MFORM in
CGI.pl

All together now...'YUCK!'

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 95640 [details] [diff] [review]
Patch against trunk, write %::MFORM only if multi-valued

This one seems to work

r=bbaetz, but I want a second reviewer to sanity check this.

Comment 16

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

Comment on attachment 95640 [details] [diff] [review]
Patch against trunk, write %::MFORM only if multi-valued

r=myk

This is the right solution, and it looks good and works.

Comment 17

[Bradley Baetz (:bbaetz)]

This has been checked in on the TRUNK.

Dave - you marked this as [wanted for 2.16.1], but its not a regression there;
the only regression part of this was trunk only. Do you still want this for the
branch?

-> patch author, in any event

Comment 18

[Bradley Baetz (:bbaetz)]

Attachment: 2.16 patch

Can I get a rubberstamping for this? I jsut removed the changes for the login
template, which doesn't exist in 2.16

Comment 19

[J. Paul Reed [:preed]]

Comment on attachment 97580 [details] [diff] [review]
2.16 patch

Don't know how valid my rubber stamp is, but it looks good.

What about 2.14.3? Was that ever checked in? Did it need to be?

Comment 20

[Bradley Baetz (:bbaetz)]

No, it wasn't, and we're not going to be fixing this sort of bug for 2.14, esp
since it took this long for someone to notice.

Checked in to 2.16

Comment 21

[Dave Miller [:justdave]]

*** Bug 183992 has been marked as a duplicate of this bug. ***