On IRC:
[17:50] Anyone encountered a situation where, with formsapi, a checkbox that's given a default value will always return that value? even if it's unchecked?

I have investigated this and found if a form has at a checkbox on it that it's default_value is set to 1, it will always return 1 even if unchecked.

This can be seen in the publishing options on a create content form, checkboxes set to 1 can not be cleared, As well as many of the checkboxes on the theme configure settings page.

This patch may not be completely correct, but the checkboxes now work and I can't find any side-effects so far.

This may also fix:
http://drupal.org/node/33700

CommentFileSizeAuthor
#27 form_def_val_4.patch946 byteschx
#22 form_def_val_3.patch915 byteschx
#15 form_def_val_2.patch727 byteschx
#14 form_def_val_1.patch727 byteschx
#13 form_def_val_0.patch1.21 KBchx
#12 form_def_val.patch728 byteschx
form-builder-default-value.patch691 bytesasimmonds

Comments

asimmonds’s picture

asimmonds commented

Just to give credit where credit's due, the IRC quote was from Eaton

chx’s picture

chx commented
Status: Needs review » Needs work

That isset is added there because of a bug eaton reported before...

eaton’s picture

eaton commented

I just cause all sorts of problems, don't I?

The problem, I think, is that in the case of checkboxes, we shouldn't have a default_value -- rather, we should have a default checkstate, and a return_value. If it's unchecked, no value should be returned at all. It's easy enough to check to see if the element is '#type' == checkbox', but I'm not sure what else that might break. Any thoughts?

Is this an underlying hole in the forms architecture, or something I'm not getting?

chx’s picture

chx commented

Eaton, sorry I mixed you with m3avrck. Well, that's a problem for tomorrow...

eaton’s picture

eaton commented

As a side note, this patch DOES fix the problem that was being encountered in http://drupal.org/node/33700 ... I'm just not sure what subtle rippling might be caused by this change.

m3avrck’s picture

m3avrck commented

Line 236: $form['#value'] = ($posted && isset($edit)) ? $edit : $form['#default_value'];

Without this isset() values from the database aren't loaded into my form by default.

Here is the code working with the above value and what $form looks like: http://pastebin.com/412620

And without it (revering the change) to:
$form['#value'] = ($posted) ? $edit : $form['#default_value']; : http://pastebin.com/412624

Relevant values to search for: 1, 1, 11, and 1 (these are loaded into the $days part of the array and set for w1_sun, w1_tue, w1_thu, w1_fri)

However, these values appear in both arrays but the input boxes in the later case aren't rendered with the values filled in by default and editable by the user.

chx’s picture

chx commented

I talked with m3avrck a bit more and there is a http://pastebin.com/412635 version of the forms array before drupal_get_form, so we can play with it. I will do, tomorrow.

m3avrck’s picture

m3avrck commented

And right before drupal_get_form(): http://pastebin.com/412635 ... as you can see the values are loaded however they don't actually get put into the input box itself unless the isset() is there.

m3avrck’s picture

m3avrck commented

Just to clarify, only are the textfields affected, the selects have the correct value selected but all of the textfields are empty unless the isset() is there.

eaton’s picture

eaton commented

I have a sneaking suspicion that the best solution is to alter radio and checkbox behavior -- the more I look at it, there should NOT be a '#default_value' for these elements. They should only have a #return_value that is or is not passed back. What they DO require is a '#checked' flag that's used when rendering the actual form itself.

asimmonds’s picture

asimmonds commented

I see what the problem with m3avrck's textfields and my patch is, if $_POST['edit'] is populated with anything before the form is built, then default_value is ignored.
My bad, I should have thought of this.

I'm leaning towards Eaton's view that checkboxes (may be radios too) require special handling.

What a can of worms :-), I'll have a think about all this before tonight.

chx’s picture

chx commented
StatusFileSize
new form_def_val.patch728 bytes

What about this?

chx’s picture

chx commented
StatusFileSize
new form_def_val_0.patch1.21 KB

I meant this. So if we are facing a checkbox (or a radio) then $posted rules.

chx’s picture

chx commented
StatusFileSize
new form_def_val_1.patch727 bytes

Removed the doc hunk from my patch (though it does no harm)

chx’s picture

chx commented
StatusFileSize
new form_def_val_2.patch727 bytes

I swear won't code this late again. But I just can't stop :)

eaton’s picture

eaton commented

+1 works

Just tried it out with a default_value'd checkbox and it works like a charm. I'm not sure if it still fixes m3avrck's issue with select boxes, but it certainly does fix the issue *I* was running into. Awesome work, chx! I was getting lost in some of the array processing there.

eaton’s picture

eaton commented
Status: Needs work » Needs review
chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community
m3avrck’s picture

m3avrck commented

patch is working for me too, wahoo! good job chx!

asimmonds’s picture

asimmonds commented

Patch works for me too, much better than my lame attempt. I've tested as many Drupal core forms as I can, with no side-effects that I can see.

Another round of thanks to chx

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

Can we make the code more readable? It looks pretty hairy, and unless you're a form API guru, you can't make sense of this. Thanks.

chx’s picture

chx commented
StatusFileSize
new form_def_val_3.patch915 bytes
chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community
adrian’s picture

adrian commented

This is the exact patch that is needed.

+1.

asimmonds’s picture

asimmonds commented

I have found a small (related) problem. (the following is with current head form.inc + patch #22)
If a checkbox is unchecked in a form, the relevant value in the returned $form_values is set to a NULL. If a NULL is passed to variable_set() (eg system_settings_form_execute()) then a 'N;' is inserted into the database variable table for that variable. (as seen under mysql 4.0)

If this variable is read back with variable_get(), then the returned value will be set to the default because a NULL is being read from the database and get_variable() uses isset() to test wether it should return the default or not.

eg A form with:

  $form['check1'] = array(
    '#type' => 'checkbox',
    '#title' => 'Test Checkbox (default checked)',
    '#default_value' => variable_get('test_checkbox1', 1),
  );

and the following in the form's _execute hook

variable_set('test_checkbox1', $values['check1']);

This form will not allow the checkbox to be left unchecked, as the 'test_checkbox1' variable will be NULL in the database.

Should the return_value in $form_values for a unchecked checkbox be NULL?

dries’s picture

dries commented

Not committing yet. Waiting for asimmonds problem to be resolved or clarified.

chx’s picture

chx commented
StatusFileSize
new form_def_val_4.patch946 bytes

I am definitely sure this will do.

asimmonds’s picture

asimmonds commented

OK, #27 passes my tests so I'll say it's good to go.

eaton’s picture

eaton commented

Works great for me, too. Very nice. The new code, while longer, is also much easier to follow as well.

m3avrck’s picture

m3avrck commented

Works for me too!

I think that was the 3 unique cases so let's commit it!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Much nicer code, easy to understand.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)