[drupal-devel] [bug] fix empty form elements

Sat May 21 16:43:49 UTC 2005

Issue status update for http://drupal.org/node/18663

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     critical
 Assigned to:  chx
 Reported by:  killes at www.drop.org
 Updated by:   chx
 Status:       patch
 Attachment:   http://drupal.org/files/issues/fix_form_2.patch (3.23 KB)

People told me that form_radios does not need this because we can
reasonably expect that a radio is checked, that's different between a
bunch of checkboxes and a bunch of radios.

form_select submits an empty array (as also told by some).

So we are back to checkbox and checkboxes which need the fix.

chx

Previous comments:
------------------------------------------------------------------------

March 10, 2005 - 03:51 : killes at www.drop.org

If you select nothing from a set of checkboxes an empty string is save
to the variables table. This is a problem if the calling code assumes,
it would get an array.
We should make it save an empty array. The problem is the form_hidden
field that we add. even if we replace form_hidden($name, 0) by
form_hidden($name, array()) it isn't going to work, because the empty
array is eaten by htmlspecialchars.

redLED found this bug and he does not like it:
02:53 < redLED> i just like things saying 'CRITICAL' in red
02:53 < redLED> it gives this warm impending doom feeling
02:53 < redLED> and is generally associated with nuclear reactors in
people's subconsciousness

Suggestions welcome.

------------------------------------------------------------------------

March 12, 2005 - 20:47 : Steven

There is no way to fix it in form_checkboxes() itself afaik, because
even without htmlspecialchars, it would still come out as "Array". You
can only create arrays indirectly through form submissions, by naming
your values "foo[]". But you can not create empty arrays this way.

So it is really up to the module which uses form_checkboxes() to handle
this. A simple is_array() should do the trick.

------------------------------------------------------------------------

March 28, 2005 - 18:27 : chx

Attachment: http://drupal.org/files/issues/checkboxes.patch (1.71 KB)

system_settings_save can not really cope with this situation. This makes
a problem with empty node options. And the fact that we have a check
everywhere we use form_checkboxes is error prone, too. Here is a patch.

If this accepted I'll comb the core for dead code. For eg. the first
three lines of taxonomy_save_vocabulary and so.

------------------------------------------------------------------------

March 28, 2005 - 18:32 : chx

Attachment: http://drupal.org/files/issues/checkboxes_0.patch (1.71 KB)

Oh NO! One strayed space! I fixed it before steven beheads me....

------------------------------------------------------------------------

March 28, 2005 - 19:36 : chx

Attachment: http://drupal.org/files/issues/checkboxes_1.patch (1.64 KB)

Among other changes, I have moved the hidden field form_checkboxes[] out
of $edit. I have included HTML code, yes, but this is a very special
thing, so it does not merit changing form_input just 'cos of this. And
you won't need to theme a hidden field...

------------------------------------------------------------------------

April 8, 2005 - 09:19 : chx

Attachment: http://drupal.org/files/issues/form_checkboxes_2.patch (1.9 KB)

killes did not like the previous version.

------------------------------------------------------------------------

April 12, 2005 - 01:15 : chx

Dries?

------------------------------------------------------------------------

April 12, 2005 - 21:15 : chx

Attachment: http://drupal.org/files/issues/form_checkboxes_3.patch (1.64 KB)

------------------------------------------------------------------------

April 12, 2005 - 21:39 : jhriggs

I have to agree with Steven that it is up to the calling code...and I
hardly think this is critical.  Code that is expecting an array from
form_checkboxes() should ensure that it is dealing with an array (i.e.
is_array()).  As for dealing with variables set through things like
system_settings_save(), the same thing applies.  The code that is
retrieving the variable should ensure that it is working with an array.

With regards to your patch, what's the difference if the calling code
has to remember to check via is_array() or remember to call
fix_checkboxes()? This method is no less error prone as it's just as
easy to forget this call as it is to check for an array.  That is
unless you are suggesting that fix_checkboxes() gets called for every
page load which seems rather crufty.

I wrote the original patch to add form_checkboxes() -- which didn't
exist -- and if I remember, this was the desired behavior if nothing
was selected. Isn't this the same behavior one would see using a
multiple form_select() if the user chooses nothing?  They should behave
the same. A single select widget is the same as a radio group, and a
multiple select widget is the same as a group of checkboxes.  They
should behave the same; the select types just take up less realestate.

------------------------------------------------------------------------

April 12, 2005 - 23:23 : chx

Attachment: http://drupal.org/files/issues/form_checkboxes_4.patch (2.06 KB)

Previous patch had index.php missing.

jhriggs, the current problem is that a form_checkboxes expected to be
an array. Currently, empty form_checkboxes is a string. That's bad.

------------------------------------------------------------------------

April 13, 2005 - 00:51 : jhriggs

-1 on this patch.

I still don't understand the problem.  What do you mean by
"form_checkboxes expected to be an array"?  Is there code in core that
assumes it is an array but it is not checked with is_array()?  If so,
let's add the missing checks. The same goes for contrib.  This seems
like swatting a fly with a sledge hammer to me.

If we do go ahead and make this change, however, then we will need to
change form_select() also (when multiple = TRUE).  Radios and single
select should be consistent, as should checkboxes and multiple select.

------------------------------------------------------------------------

April 23, 2005 - 13:35 : chx

Attachment: http://drupal.org/files/issues/fix_form.patch (4.71 KB)

See Gordon's complaint
http://lists.drupal.org/archives/drupal-devel/2005-04/msg00934.html

------------------------------------------------------------------------

April 23, 2005 - 13:38 : chx

Forgot to change the title.

------------------------------------------------------------------------

April 23, 2005 - 13:46 : chx

It seems that it's not obvious why this is necessary. The calling code
can not always handle itself the situation if the save is done by
system_save_settings. The edge case is: I set up something, so
variable_get no longer falls to its default value, later I decide to
empty it. If we do not do something like this patch,
system_save_settings can not know there is something to be set to
0/array() because it is just not in $_POST.

------------------------------------------------------------------------

April 23, 2005 - 15:23 : chx

Attachment: http://drupal.org/files/issues/fix_form_0.patch (4.72 KB)

Small bugfix.

------------------------------------------------------------------------

April 23, 2005 - 16:53 : chx

doh. still has serious problems when the name is an array. stay tuned.

------------------------------------------------------------------------

April 24, 2005 - 11:13 : chx

Attachment: http://drupal.org/files/issues/fix_form_1.patch (4.97 KB)

Recursive version.

------------------------------------------------------------------------

April 24, 2005 - 14:51 : Dries

Patch looks good but I would add some phpdoc to document what and why we
are doing this.