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.
Comment | File | Size | Author |
---|---|---|---|
#33 | node.module.emptyform.patch | 657 bytes | David Lesieur |
#25 | fix_form-4.6.3.patch | 3.54 KB | tostinni |
#20 | fix_form_3.patch | 3.24 KB | chx |
#19 | fix_form_2.patch | 3.23 KB | chx |
#17 | fix_form_1.patch | 4.97 KB | chx |
Comments
Comment #1
Steven CreditAttribution: Steven commentedThere 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.
Comment #2
chx CreditAttribution: chx commentedsystem_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.
Comment #3
chx CreditAttribution: chx commentedOh NO! One strayed space! I fixed it before steven beheads me....
Comment #4
chx CreditAttribution: chx commentedComment #5
chx CreditAttribution: chx commentedAmong 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...
Comment #6
chx CreditAttribution: chx commentedkilles did not like the previous version.
Comment #7
chx CreditAttribution: chx commentedDries?
Comment #8
chx CreditAttribution: chx commentedComment #9
jhriggs CreditAttribution: jhriggs commentedI 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.
Comment #10
chx CreditAttribution: chx commentedPrevious 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.
Comment #11
jhriggs CreditAttribution: jhriggs commented-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.
Comment #12
chx CreditAttribution: chx commentedSee Gordon's complaint http://lists.drupal.org/archives/drupal-devel/2005-04/msg00934.html
Comment #13
chx CreditAttribution: chx commentedForgot to change the title.
Comment #14
chx CreditAttribution: chx commentedIt 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.
Comment #15
chx CreditAttribution: chx commentedSmall bugfix.
Comment #16
chx CreditAttribution: chx commenteddoh. still has serious problems when the name is an array. stay tuned.
Comment #17
chx CreditAttribution: chx commentedRecursive version.
Comment #18
Dries CreditAttribution: Dries commentedPatch looks good but I would add some phpdoc to document what and why we are doing this.
Comment #19
chx CreditAttribution: chx commentedPeople 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.
Comment #20
chx CreditAttribution: chx commentedComment #21
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #22
TDobes CreditAttribution: TDobes commentedWhy do we now have both a _fix_checkboxes() and fix_checkboxes() function? Shouldn't we name these something a little more intuitive? Without poking through the code, I have no idea what _fix_checkboxes is for and why it's different than fix_checkboxes. It really deserves some phpdoc too. Any sort of more-pleasant variable naming in _fix_checkboxes() would be much appreciated as well... It's easy to get lost in that array1/array2/single letter variable business pretty fast. Sorry for nit-picking, but most of the Drupal core code is far more readable than this.
Comment #23
(not verified) CreditAttribution: commentedComment #24
mr700 CreditAttribution: mr700 commentedIt's strange that this problem still appears in drupal 4.6.3 (August 15, 2005); 4.6 CVS, and CVS Head and has been marked fixed on May 21, 2005. The last patch (fix_form_3.patch from comment 20) still applies with little fuzziness and fixes the problem.
Comment #25
tostinni CreditAttribution: tostinni commentedThe problem has been fixed in HEAD as Dries said in #21 HEAD refer to the future 4.7 version.
Here is the updated patch that apply to 4.6.3
Comment #26
Dries CreditAttribution: Dries commentedCan someone test this first?
Comment #27
mr700 CreditAttribution: mr700 commentedFor me it works on a site with 2000 - 3000 visitors/day. Maybe it's a good idea to also add this sql statement in updates.inc?
update variable set value="a:0:{}" where name like "node_options_%" and value = "s:1:\"0\";";
Comment #28
dopry CreditAttribution: dopry commentedSetting to won't fix, unless someone tests the patch.
1) doesn't seem to have an adverse effect.
2) in a way is a nit picky issue.
3) altering the behavior of form_checkboxes 5 revisions into the release may cause more harm than good.
4) is_array is probably far less costly performance wise than this patch.
feel free to change the status if I am incorrect. I'm just cleaning out old issues.
Comment #29
dopry CreditAttribution: dopry commentedoh yeah, actually setting to won't fix.
Comment #30
bradlis7 CreditAttribution: bradlis7 commentedOk, I don't know how to do this patch thing, but couldn't this be fixed simply by adding the following:
In my node.module file, it's line 1314. It's directly after "$node_options = ..." in "function node_form(...)"
I'm going to set this to active, because I think this will fix the problem. Maybe it's fixed in 4.7, but many of us are still stuck in 4.6, and I'd like to get this dealt with.
Comment #31
bradlis7 CreditAttribution: bradlis7 commentedThere's also a problem in the "node_validate($node)" function:
It's line 1248 in my node.module.
Comment #32
Zen CreditAttribution: Zen commentedThis has been fixed in 4.7 HEAD.
-K
Comment #33
David Lesieur CreditAttribution: David Lesieur commented#30 works for me. I am attaching the corresponding patch, tested in 4.6.6.
This is not the general solution, but fixes this related bug.
This is fixed in 4.7 but might still be an issue for 4.6 users.
Comment #34
(not verified) CreditAttribution: commented