It appears that radio/checkboxe fields that are set to be 'required' in the CCK type are not being evaluated for input within this module's context. To reproduce:

1) Create a nodeprofile type containing both a required text field, and a required radio field,
2) Integrate it with registration.
3) Register a user, without filling in the required fields in the nodeprofile.

If reproduced properly, you should receive a validation error saying the text field is required - but notice there is no such validation performed for the radio field. Despite the required field not being filled in, the nodeprofile is saved, and the user is registered.

4) Go to Administer > Content and try to edit and save the profile that was just created

(Note, this is now *outside* of the context of the nodeprofile module) If reproduced properly, you will find that here, in the regular content edit form returns a validation error (as expected) for the required radio field.

My suspicion is that the issue really lies in subform_element, but I haven't looked deeply into it yet.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smooshy’s picture

I have this same problem. Has anyone come up with a solution?

riverfr0zen’s picture

Until this gets fixed, my quick solution (and sucky) solution was to change radios/checkboxes to select fields.

fago’s picture

Project: Node Profile » Subform Element
Version: 5.x-1.2 » 5.x-1.3

hm, I can reproduce the problem. It's a bug of the subform element module. Unfortunately, it's quite time intensive to go after this, and currently I've not much time for it - so I'll look at it when I find the time for it..

Perhaps, you could help me a bit with this and test if things change with other versions of the subform element module.

qingdao’s picture

im having the same problem, will this bug be fixed soon ? (next some weeks)

bjacob’s picture

Subscribing... Any updates yet?

qingdao’s picture

to realize "required check" open the file "optionwidgets.module" (modules/cck) and replace line 117
'#required' => FALSE,
with
'#required' => $field['required'],

after this it will work

unfortunatelly the checkboxes are not marked with the red dot, which tells the user that this input is required, but at least the validation is working ...

rlnorthcutt’s picture

I made the change to the "optionwidgets.module" above, and now the required radio button DOES show the red *... but it doesn't require the radio to be selected...

It seems like its not validating.

ron

idmacdonald’s picture

I can confirm this bug. It seems to occur when using the integer field type and either of the checkbox widgets (single or multiple). It does seem to be a subform problem, because when editing the node normally, everything works as expected. However, when editing the node via nodeprofile (and subform element), this bug crops up. The value in the database is not changed when I uncheck a box that was previously checked. So, somehow '0' or null input values are not being passed correctly by the subform module.

If I set the 'off' value for the checkbox to a non-zero integer value, then the bug doesn't occur. So, it would seem to be a form of the PHP zero problem.

-Ian

qingdao’s picture

this code changes affect only for the checkbox and not the radio box

rjleigh’s picture

OK, after some painful hours of tracing and examination, I figured out why this is happening.

This all happens in form.inc.

Right at the top of 'drupal_prepare_form' is the following code:

  if (!isset($form['#post'])) {
    $form['#post'] = $_POST;
    $form['#programmed'] = FALSE;
  }
  else {
    $form['#programmed'] = TRUE;
  }

The actual error code should be set in the '_form_validate' function, but it won't get set if there's no '#needs_validation' element.

The '#needs_validation' element should get set in the 'form_builder' function towards the bottom, but it won't happen because it's in a conditional statement that tests for $form['#programmed'], which is TRUE because subform set the post.

Now, HOW do we fix it? Easy to do with a patch to form.inc, but that's lame.

I was looking at adding a $form['#after_build'] function, since I don't see a hook that will work, but haven't gotten it to work yet.

Any ideas?

rconstantine’s picture

subscribing

ellen-dlma’s picture

subscribing

tanjerine’s picture

Title: Radio/checkboxes set to 'required' are not validated » subscribing

subscribing

bjacob’s picture

Title: subscribing » Radio/checkboxes set to 'required' are not validated

changed title ;)

izmeez’s picture

I have noticed the same problem in the Drupal 6.x Profile module where I have one check box that is required. When the user first creates an account if they ignore this field they are not reminded that it needs to be completed. When the user later returns to edit their profile they do receive a reminder that the field must be completed. Is there a patch that would work for this? Thanks,

Izzy

kenorb’s picture

The same problem.
subscribing
Tested on Drupal 5.7 + CCK 1.6

kenorb’s picture

Status: Active » Needs review
FileSize
463 bytes

Made a simple patch.
Problem was that #value wasn't empty() after nothing was selected.

izmeez’s picture

Is this patch also for Drupal v6.2 ?

Thanks,

Izzy

bcn’s picture

subscribing

xxparanormalxx’s picture

subscribing

smooshy’s picture

the patch doesn't appear to work for my situation. When I edit it through the pageroute page, the value never gets set to 'off'

TyraelTLK’s picture

Is the same problem that affects Profile core module in drupal 6.2?
http://drupal.org/node/259292

Chill35’s picture

Is this a problem in form.inc ? The patch is for form.inc.

If so, this should not be associated with the module Subform Element anymore.

This problem is in Drupal 6.

ejakila19’s picture

Project: Subform Element » Drupal core
Version: 5.x-1.3 » 6.x-dev
Component: Code » forms system

I had this problem with radio buttons and I was surprised to find out that there is no validation function in optionwidgets.module for required fields.

My solution was to add the case for 'validate' in the optionwidgets_widget function:

function optionwidgets_widget($op, &$node, $field, &$items) {
.
.
.
     <new code>
     case 'validate' :
          _optionwidgets_widget_validate($node, $field, $items);
          break;
     </new code>
     ...
     case 'prepare form values':
          $options = _optionwidgets_options($field, $node);
.
.
.

then I added the following function to optionwidgets.module:

function _optionwidgets_widget_validate($node, $field, $items) {
     if (!$field['required']) {
          return;
     }
	
     switch ($field['widget']['type']) {
          case 'options_buttons' :
               if (!$items['key']) {
                    form_set_error($field['field_name'], t('@field is required.', array('@field' => $field['widget']['label'])));
               }
          break;
     }
}

This should fix the case for radio buttons, but it doesn't address checkboxes. To do so, you must add "case 'checkboxes' : ...code..." to the validation function just as I have done for 'options_buttons'

I hope this helps somebody

Robert Castelo’s picture

I've created a generic solution here:

http://drupal.org/project/checkbox_validate

Just install the module and all checkboxes in every form display and behave the way they should.

I like this solution because it means developers don't need to add any extra validation code to their modules to make up for the bug in core, and once core is fixed this module can be uninstalled.

If your module relies on required checkbox fields be sure to make this module a dependency.

webchick’s picture

See also #259292: Required radios/checkboxes are not validated (D6) which I marked a duplicate of this bug.

Chill35’s picture

Wonderful, Robert, thank you :-)

bjacob’s picture

Hi Robert,

thanks for your patch. Can you provide a patch for Drupal 5.x as well?

@ejakila19: I've applied your solution. Here's the code for a single on/off checkbox:

function _optionwidgets_widget_validate($node, $field, $items) {
...

    case 'options_onoff':
      if (!$items['keys']) {
        form_set_error($field['field_name'], t('The field @field is required.', array('@field' => t($field['widget']['label']))));
      }
      break;
}

Thanks a lot, Bjoern

Robert Castelo’s picture

Hi bjacob, I think the module should also work on Drupal 5, maybe .info file might need adjusting to install properly.

deviantintegral’s picture

#17 doesn't work for me either. This even effects simple forms with a required checkbox for me.

--Andrew

Dave Reid’s picture

Does anyone know if this been fixed in D7?

Antinoo’s picture

-12 days, and this bug will be 1 year old :-)

:-(

webchick’s picture

Then someone better get on reviewing the patch and providing results, and then marking it RTBC. :P

Dave Reid’s picture

Marked #224012: required fields don't work correctly as a duplicate of this issue.

misiu9’s picture

Version: 6.x-dev » 6.3
Priority: Normal » Critical
Status: Needs review » Active

Is this problem fixed in any recent version of drupal???

Im using drupal 6.3 and the problem still exists with radio buttons and check boxes which are marked as required.

During registration the required check boxes are being ignored.

Thanks for any help

Michael

Dave Reid’s picture

Version: 6.3 » 7.x-dev
Priority: Critical » Normal

No, this has not been fixed yet and for all I know, is still present in 7.x where it will need to be fixed first, and then backported to 6.x and 5.x. This is also not a critical patch because it does not need to be fixed immediately because the software is not usable at all.

sun’s picture

Title: Radio/checkboxes set to 'required' are not validated » Required radios/checkboxes are not validated
Version: 7.x-dev » 6.x-dev

I am not able to replicate this bug in 7.x. Setting #required => TRUE on radios and checkboxes properly returns a validation error.

Dave Reid’s picture

Thanks for checking sun!

deviantintegral’s picture

Version: 6.x-dev » 7.x-dev
FileSize
756 bytes

Actually, I can replicate this on HEAD. It only happens with the checkbox type - in fact, I've never had a problem with checkboxes / radios. The #259292: Required radios/checkboxes are not validated (D6) is actually a better title for this issue.

Here is a pre-made module to easily test this. I will try and come up with a simpletest for this issue after lunch :)

deviantintegral’s picture

Assigned: Unassigned » deviantintegral

I have a patch for it which came about working on how to test this issue. I'll post both when I'm done the test.

deviantintegral’s picture

Status: Active » Needs review
FileSize
4.03 KB

Here is the patch along with the test. The patch does two things;

  1. Modifies _form_validate() to notice the case where #type == 'checkbox', and if that's the case, don't allow #value to be zero. I believe this bug was introduced in #117748: Trim required fields on validate.
  2. Modifies theme_checkbox() so that required checkboxes show the required state. Currently, #title is unset in theme_checkbox, which means that the required state is never added (as there's no label element for single checkboxes). As far as I can tell from the CVS logs, this function never properly themed required checkboxes. As anyone overriding this theme function may have started with this code, I'm going to suggest that a special note of this be made in the release notes for themers.

This passes all tests currently.

Gábor Hojtsy’s picture

Dries’s picture

The code looks good to me, although that check in form.inc is getting a little nasty.

The change to theme_checkbox I'm not 100% sure about.

Thanks for adding a test!

I'll let some more people review this patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

deviantintegral’s picture

Status: Needs work » Needs review

I was just able to install HEAD with this patch applied, so flipping back to CNR.

Damien Tournoud’s picture

Status: Needs review » Needs work

Please leave this poor check alone, it totally makes sense. The issue is in form_type_checkbox_value()... but there could be more than one bug there.

Damien Tournoud’s picture

Assigned: deviantintegral » Unassigned
Status: Needs work » Needs review
FileSize
5.83 KB

form_type_checkbox_value() had a bug, that #229129: System module page *seriously* broken tried to fix totally the wrong way.

The bug is that

return !empty($edit) ? $form['#return_value'] : 0;

should be:

return !empty($edit) ? $form['#return_value'] : NULL;

Because a non-checked checkbox should not return any value, not the '0' value. This breaks both the logic that is triggered later on to "set the default value if no value was found" (which #229129: System module page *seriously* broken tried to fix) and the "a required checkbox is not validated because even a non-checked checkbox has a value of '0'" (which this patch tried to fix the wrong way).

The attached patch:
- makes the correct fix,
- reverts the silly unit tests that have been committed in #314283: TestingParty08: Disabled checkboxes need tests, in an effort to make sure that the crappy fix was working,
- adds a new test,
- add the fix to theme_checkbox, that IMHO makes sense (there is a big of redundant code with theme_form_item(), but we can't really do anything else because we need to output the checkbox *inside* the label tag)

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

I can confirm the results of the testing bot. This still needs work.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
11.6 KB

Now that I think about it at a non-crazy hour, we really do need that special #disabled handling because there is no way to differentiate between a "non-checked checkbox" and a "disabled checkbox".

So I rewrote and documented form_type_checkbox_value this way:

    if (isset($edit)) {
      // A value is passed by the browser: the checkbox is on.
      return $form['#return_value'];
    }
    else {
      if (!empty($form['#disabled'])) {
        // Disabled checkbox values are not passed by the browser,
        // use default instead.
        return $form['#default_value'];
      }
      else {
        // The checkbox is off.
        return '';
      }
    }

I also added a unit test of checkboxes, which could be extended at a later stage to test other widget types.

100% on my development environment, let's see what the bot thinks.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
11.51 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

I can't reproduce the testing failures.

geerlingguy’s picture

D'oh! Just ran into this problem in Drupal 6. Kind of annoying bug, as I'd like to have an "I read the Privacy Policy blah blah" checkbox on the user registration form, but without this validation, it doesn't have to be checked...

kenorb’s picture

#55 -> #25

geerlingguy’s picture

@ kenorb - ah, thanks. I saw that, but my brain must've been turned to 'stupid' mode and I didn't regard it as useful to me. Installing now!

bcn’s picture

Status: Needs work » Needs review
FileSize
11.3 KB

Re-roll of the patch from #52.

Status: Needs review » Needs work

The last submitted patch failed testing.

Jhef.Vicedo’s picture

subscribing

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
11.31 KB

Let's try again.

Status: Needs review » Needs work

The last submitted patch failed testing.

seanr’s picture

Priority: Normal » Critical

Why are we considering this to be a normal bug instead of critical? This effects every module that uses the checkbox field, and as geerlingguy noted, it represents a serious liability in certain cases.

In my particular case, it is actually causing a LEGALLY REQUIRED (as in federal election law!) checkbox not to validate properly. I now have to write extra code into my module that I would never have expected to need because Drupal core isn't performing the validation as advertised.

Let's please agree that this bug is a little more serious than your run of the mill display issue.

webchick’s picture

So how about re-rolling and testing the patch, rather than re-iterating how much this needs to be fixed? :)

Antinoo’s picture

seanr’s picture

Webchick - I could do that for D6, but not D7 as I don't currently have PHP 5.2. Also, the testing errors give far too little information for me to successfully re-roll the patch. Why can we not see exactly which assertions failed? Failed 2 out of 50 tests is useless because I have no idea which the 2 are. BTW, my comment was largely a response to the person who previously downgraded this issue to normal - they seem not to understand the potential impact of this issue.

Antinoo - that is helpful, but it's a bad precedent to have to rely on a contributed module to fix a big bug in core. ;-) Besides, how many users will never even be aware this bug exists until after they've already gotten a ton of bad form submissions?

geerlingguy’s picture

@ seanr / #66: can you set up a local LAMP/MAMP/WAMP environment, install D7 on top of PHP 5.2.x, then run the tests locally using the testing module? If you do that, you can see precisely which tests are failing.

andypost’s picture

subscribe

WiredEscape’s picture

subscribe

netron’s picture

came across this bug on a D6 install i'm building for the client.

had to install the checkbox validate module

http://drupal.org/project/checkbox_validate

just something to be aware of if your client requires a legal "i agree to these terms and conditions" or a "i am over 16 years old" user register checkbox.

davea’s picture

subscribe

webchick’s picture

I'm inventing a tag for this. Would really appreciate even one, just one! of the people here having this bug taking a few minutes to re-roll and review this patch rather than repeatedly saying you have the same problem. We know. We need people to work on the patch to get it fixed.

mgifford’s picture

Ack. This isn't a hard thing to do. Doesn't take 2-3 weeks to re-roll a patch.

Subscribe.

luxx’s picture

subscribing

bcn’s picture

FileSize
10.47 KB

here's a re-roll...

bcn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

joep.hendrix’s picture

subscribing

andypost’s picture

A lot of tests broken, patch needs re-roll

halloffame’s picture

Subscribing... I gotta stick with checkbox_validate module for the meantime.

jromine’s picture

FileSize
11.41 KB

re-rolling this patch.

geerlingguy’s picture

Status: Needs work » Needs review

Setting status for the testbot.

Status: Needs review » Needs work

The last submitted patch failed testing.

jromine’s picture

Status: Needs work » Needs review
FileSize
13.79 KB

try again

Status: Needs review » Needs work

The last submitted patch failed testing.

jromine’s picture

Status: Needs work » Needs review
FileSize
12.78 KB

Updated patch

jcmarco’s picture

Wow! That's great! Congratulations!

This is one of the historic bugs in drupal for long time!

I have backported it to D6.13 and it works also great.

I was testing it with forms and even with the legal module without the checkbox_validate module that was patching this bug for long time.
Now, you always see the required field asterisk and stop you to send the form without the required field.

Status: Needs review » Needs work

The last submitted patch failed testing.

jcmarco’s picture

Status: Needs work » Needs review

Umm! something wrong with the bot, that is a backporting to D6.13 to ease testing it.
Sorry, I didn't read the insurance contract letter size in the attachment form.

jcmarco’s picture

mgifford’s picture

Patch needs to be built from root not from the includes directory. Patch #90 is going to fail too unfortunately.

But the code also doesn't work against core:

bash-3.2$ cvs update includes/form.inc
cvs update: warning: `includes/form.inc' was lost
U includes/form.inc
bash-3.2$ patch -p0 < form_checkbox_validate-D6.patch
can't find file to patch at input line 3
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|--- form.inc.orig 2009-07-22 14:21:19.328125000 +0200
|+++ form.inc 2009-09-03 11:15:20.234375000 +0200
--------------------------
File to patch: includes/form.inc
patching file includes/form.inc
Hunk #1 succeeded at 787 (offset 112 lines).
Hunk #2 FAILED at 1285.
Hunk #3 FAILED at 1990.
Hunk #4 succeeded at 2064 with fuzz 2 (offset 127 lines).
2 out of 4 hunks FAILED -- saving rejects to file includes/form.inc.rej

jcmarco’s picture

Yes, it was wrong. First core patch rookie.
Once again, this is just a backporting to D6 from #86

mgifford’s picture

Ok, patch in #86 applies nicely.

What's stopping this from becoming RTBC?

What testing should be done? I'm running Simpletest now, I'm assuming it's all under the Form API

AndraeRay’s picture

This is great, It's awesome that this finally came together. I used the patch in post 92 For my Drupal 6 and it works great. Special thanks are definitely due for all the hard work.

mgifford’s picture

The patch in #92 is for Drupal 6.

The patch in #86 is for Drupal 7.

We need more folks to review the patch for D7 before we can get it into core. At that point we can consider bringing this back to D6.

So who is going to do the final review of the D7 patch and mark it RTBC?

seanr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
863 bytes

I tested the patch in #86 and can confirm it works as expected. I created a simple module to test it and have attached it for anyone else who'd like to give it a try. The same module should work for Drupal 6 with an edit to the .info file.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

seanr’s picture

Status: Needs work » Reviewed & tested by the community

Bot appears screwed up because of #87. Should the patch from #86 be resubmitted?

mgifford’s picture

I think it will be clear when the patch is being reviewed.

chx’s picture

The patch reuses the integer 0 which already is unsupported as a checkboxes key. Good trick.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Yay! Sign-off from the Form API maintainer! This is really close. I have some minor nits, mostly related to comments. Since this part of the code has been broken for eons, it'd be nice to document the way it's supposed to work once and for all.

+++ includes/form.inc	23 Aug 2009 16:26:03 -0000
@@ -754,7 +754,8 @@
+      // Unchecked checkbox has #value of numeric 0 (for now).

"for now"? I don't understand what that means. Please clarify.

+++ includes/form.inc	23 Aug 2009 16:26:03 -0000
@@ -1273,11 +1274,18 @@
+    if (!empty($element['#disabled'])) {
+      // Disabled controls are never successful.
+      // Always return default.
+      return $element['#default_value'];

Here again, I don't understand what that means. Successful? Meaning it's not validated, or..?

+++ includes/form.inc	23 Aug 2009 16:26:03 -0000
@@ -1273,11 +1274,18 @@
+      // Successful checkboxes are present with a value (possibly '0').
+      // http://www.w3.org/TR/html401/interact/forms.html#successful-controls
+      // For an unchecked checkbox, we should return '' or maybe NULL, but for
+      // now we return numeric 0 since other core modules use a numeric database 
+      // column for a checkbox value and assume its value will be 0 or 1.
+      return isset($input) ? $element['#return_value'] : 0;

Let's clean up these comments. They sound wishy-washy. Either we are returning zero or we're not, and we should explain why.

+++ includes/form.inc	23 Aug 2009 16:26:03 -0000
@@ -1957,17 +1965,20 @@
+  $t = get_t();
...
-    $checkbox = '<label class="option" for="' . $element['#id'] . '">' . $checkbox . ' ' . $element['#title'] . '</label>';
+    $required = !empty($element['#required']) ? ' <span class="form-required" title="' . $t('This field is required.') . '">*</span>' : '';
+    $checkbox = '<label class="option" for="' . $element['#id'] . '">' . $checkbox . ' ' . $element['#title'] . $required . '</label>';

This is really weird. None of the other element theme functions do this, no? Why are we doing it here?

+++ includes/form.inc	23 Aug 2009 16:26:03 -0000
@@ -2021,7 +2032,7 @@
-          '#default_value' => isset($value[$key]),
+          '#default_value' => isset($value[$key]) ? $key : NULL,
@@ -2134,7 +2145,7 @@
-            '#default_value' => isset($value[$key]),
+            '#default_value' => isset($value[$key]) ? $key : NULL,

Wait. NULL? I thought we returned 0? Why has this treatment not been given to other checkbox fields?

+++ modules/simpletest/tests/form.test	23 Aug 2009 16:05:14 -0000
@@ -72,42 +83,33 @@
+    $expected_values = array(
+      'disabled_checkbox_on' => 'disabled_checkbox_on',
+      'disabled_checkbox_off' => '',
+      'checkbox_on' => 'checkbox_on',
+      'checkbox_off' => '',
+      'zero_checkbox_on' => '0',
+      'zero_checkbox_off' => '',
+    );

Great tests! Awesome to have this in code so Drupal 7 is hopefully the /last/ release where we break checkboxes. :)

-9 days to code freeze. Better review yourself.

jromine’s picture

I'll cleanup the comments, and re-roll the patch if needed.

An unchecked checkbox value should be represented by NULL, but I ran into issues elsewhere with this. Returning numeric 0 as the value of an unchecked checkbox is a compromise that fixes the issue at hand, and also doesn't break things elsewhere.

In form_process_checkboxes() and form_process_tableselect():

+++ includes/form.inc 23 Aug 2009 16:26:03 -0000
@@ -2021,7 +2032,7 @@
-          '#default_value' => isset($value[$key]),
+          '#default_value' => isset($value[$key]) ? $key : NULL,
@@ -2134,7 +2145,7 @@
-            '#default_value' => isset($value[$key]),
+            '#default_value' => isset($value[$key]) ? $key : NULL,

We're looking at a set of related checkboxes, and iterating through #options. $key could be (numeric) 0 for the first item. NULL for #default value represents the checkbox being unchecked. This matches how this is handled in form_process_radios().

I think the special theme handling is because checkboxes don't have a label above the form control.

jromine’s picture

Status: Needs work » Needs review
FileSize
12.69 KB

patch attached

chx’s picture

Status: Needs review » Reviewed & tested by the community

I think I like this :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
7.09 KB

Think this brings this patch up to the CVS.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

FileSize
11.13 KB

Hmm.. Rolled that patch wrong (more coffee in me now). Hopefully that's the problem, but these are the 9 fails.

Found the Submit button Other form.test 94 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Found the requested form fields at form-test/checkbox Other form.test 94 FormsAPIUnitTest->testCheckBoxProcessing() Fail
A required checkbox is actually mandatory Other form.test 95 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Checkbox 'disabled_checkbox_on' returns expected value (expected: 'disabled_checkbox_on', got: '<') Other form.test 111 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Checkbox 'disabled_checkbox_off' returns expected value (expected: '', got: '<') Other form.test 111 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Checkbox 'checkbox_on' returns expected value (expected: 'checkbox_on', got: '<') Other form.test 111 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Checkbox 'checkbox_off' returns expected value (expected: '', got: '<') Other form.test 111 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Checkbox 'zero_checkbox_on' returns expected value (expected: '0', got: '<') Other form.test 111 FormsAPIUnitTest->testCheckBoxProcessing() Fail
Checkbox 'zero_checkbox_off' returns expected value (expected: '', got: '<') Other form.test 111 FormsAPIUnitTest->testCheckBoxProcessing() Fail

mgifford’s picture

Status: Needs work » Needs review

not quite enough coffee to change status to needs review.

Status: Needs review » Needs work

The last submitted patch failed testing.

elarifr’s picture

Hello
In drupal 6.14 i tried also checkbox #element_validate in author_contact module and it seems the function is not called while working with textfiled
it seems not only #required is buggy in checkbox
or maybe it is me :(

here are some code i played with just to see if it will display form_set_error

    $form['sendercheckbox1'] = array(
        '#title' => t(variable_get('authorcontact_form_checkbox1', 'I Agree')),
        '#type' => 'checkbox',
        '#return_value' => "Ok",
        '#required' => true ,
	'#element_validate' => array('authorcontact_form_checkbox1_validate'),
    );

and

//to check element_validate not working with checkbox 
//function authorcontact_form_name_validate($form_id, $form_values) {
//    //check the email address is valid
//    if(isset($form_values['values']['sendername'])) {
//        if (!valid_email_address($form_values['values']['senderemail'])) {
//            form_set_error('name', t('The email address you entered is not valid. '. variable_get('authorcontact_form_checkbox1', '') ));
//		}
//	}
//}
function authorcontact_form_validate($form_id, $form_values) {
    //check the email address is valid
    if(isset($form_values['values']['senderemail'])) {
        if (!valid_email_address($form_values['values']['senderemail'])) {
            form_set_error('email', t('The email address you entered is not valid'));
	}
    }
}
function authorcontact_form_checkbox1_validate($form_id, $form_values) {
    //check the mandatory checkbox is checked
//    if($authorcontact_form_checkbox1 != "itmustfail") {
        if($form_values['values']['sendercheckbox1'] != "Ok") {
            form_set_error('', t('You must check the box authorcontact_form_checkbox1. '. variable_get('authorcontact_form_checkbox1', '') ));
        }
//    }
sun’s picture

Status: Needs work » Needs review
FileSize
12.67 KB

Re-rolled #103 against HEAD + cleaned up the code.

bcn’s picture

Status: Needs review » Reviewed & tested by the community

rtbc?

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

YAY! Latest patch addresses all of my previous comments. It's with extreme excitement that I have Committed to HEAD!

This needs to be back-ported to D6 (and I think D5) now.

yched’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Active

This broke option widgets, which I'm currently cleaning and rewriting tests for.

Steps to reproduce:
- Create a field of type 'List (numeric)', with widget 'Radios / Checkboxes'
Allowed values:

0|Zero
1|One
2|Two

Number of values: illimited
- Create a new node: the 'Zero' checkbox comes out as checked (should be unchecked like the others, we're creating a new node)
- Save the node with 'Zero' and 'One' checked. On node view, you see that both values were saved.
- Edit the node: only 'One' comes back as checked ('Zero' should be checked too)

Is there some special tricks to apply to get '0' recognized as a valid value in checkboxes #options now ?

webchick’s picture

Bleh. :(

Looks like we need even more test coverage to be assured this is working. Could someone please work yched's reproduction steps into a test case?

chx’s picture

0 was never a valid checkboxes value. That's documented:

The #options array can not have a 0 key, as it would not be possible to discern checked and unchecked states.

that stands for four years now. That's not new.

yched’s picture

"0 was never a valid checkboxes value"
CCK optionwidgets have been making use of this from 4.7 until current core's options.module - and, er, maybe it was not supported, but it worked until yesterday.

This mention in the FAPI reference has been added somewhere during D6 dev cycle, but not mentioned in http://drupal.org/update/modules/5/6. So I guess we just missed that fact, and kept a working but 'invalid' feature for 3 years.

OK, I guess options.module will need to hack a translation of '0' options into '_0' or something...

chx’s picture

That worked? No way. Really, that's not possible, can you show me a Drupal page with checkboxes page that has 0 among the indexes and you can save with 0 ticked off?

yched’s picture

No example online, sorry. But you can see this working in CCK D6 by importing the attached field export.

+ I had my 'checkboxes' widgets tests for D7 (for the patch I'm working on and was about to post) work with yesterday's core and the field described in #115.

sun’s picture

The option keys should be strings.

In http://api.drupal.org/api/function/list_allowed_values_list/7, $key is a string until

    $allowed_values[$key] = (isset($value) && $value !== '') ? $value : $key;

so PHP seems to auto-convert "0", "1", etc into an integer. Is there any way to prevent this?

yched’s picture

re sun: Yes, I tried that as well, but PHP does seem to auto cast numeric strings in array keys into actual ints.
No way to keep string '0' as an array key.

sun’s picture

Status: Active » Needs review
FileSize
2.42 KB

This works in manual testing.

yched’s picture

No test bot right now, it seems, but locally this gives errors on single checkbox in "Form element validation" tests:

Checkbox 'checkbox_off' returns expected value (expected: '', got: 'checkbox_on') in form.test line 150
Checkbox 'zero_checkbox_off' returns expected value (expected: '', got: '1') in form.test line 150

yched’s picture

(forgot to say that it does fixes 'checkboxes' widgets using 0 as one of the options, though)

yched’s picture

The failures boil down to: with patch #123, the $form_state['values'] for an unchecked checkbox end up as the #default_value instead of 0.

yched’s picture

FileSize
2.8 KB

Dunno why it didn't get reported here, but test report for #123 is at http://qa.drupal.org/pifr/test/19142
14,684 pass(es), 281 fail(s), and 149 exception(es).

Attached patch uses '' instead of NULL for empty checkbox value. Solves the errors mentioned in #125-126, but still has the drawback of breaking many code, for instance code that inserts the checkbox form value directly in an int column in a table (I would have expected FALSE would work better, but turns out it's also rejected for int columns).

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

I posted #635202: Remove #process pattern from option widgets (refactors + truly tests option widgets).
The patch uses a '_0' placeholder for '0' options. We can remove it if/when this gets sorted out, but for now it unbreaks option widgets.

webchick’s picture

I guess we need to roll this back then? :\

jlain’s picture

subscribing

Status: Needs work » Needs review

jlain requested that failed test be re-tested.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

What happened from #112 to #127 to get so many failed tests?

I thought this was basically a done deal!

yched’s picture

mgifford: #112 or similar has already been committed. The followup patches are attempts to make checkboxes support '0' as a valid option again.

mgifford’s picture

Ahh.. Ok. That's god to know.

c960657’s picture

I just created #654796: Identify fix bugs with checkbox element. Not sure if that is a duplicate of this.

Reg’s picture

subscribe

Shai’s picture

subscribe

sun’s picture

So do we mark this as fixed in favor of #654796: Identify fix bugs with checkbox element ?

andypost’s picture

I think that issue should be backported because it's a bug

yched’s picture

As a D7 issue, yes, the followup should be fixed by #654796: Identify fix bugs with checkbox element.

Not sure if a D6 backport is possible.

tstoeckler’s picture

Version: 7.x-dev » 6.x-dev

Let's see.

webchick’s picture

Version: 6.x-dev » 7.x-dev

Well, you don't want a back-port of this, because it's still buggy.

Status: Needs work » Needs review

Re-test of form.inc_3.patch from comment #17 was requested by podarok.

Bilmar’s picture

subscribing

dman’s picture

#127: form_checkboxes_0.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, form_checkboxes_0.patch, failed testing.

sun.core’s picture

Priority: Critical » Normal
Status: Needs work » Fixed

This patch was actually committed, we just don't want the same patch to be backported to D6. Please create a new issue for D6. Or not.

josepvalls’s picture

So, where is the D6 issue?
I can only find issued marked as duplicate.

Cyberwolf’s picture

Subscribing.

ginc’s picture

riverfr0zen’s picture

thanks, guys

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

markosaurus’s picture

Version: 7.x-dev » 6.16
Category: bug » support

I know this has been closed for a while now, but I thought people who had been working on this might be interested and I didn't know where else to post. This relates more to the "Checkbox Validate" module than to the actual bug, but since some people (including myself) have used this as a potential fix, I thought it relevant.

I had encountered this problem in D6.16 as other people have when creating a checkbox for T&C's on a site I'm developing.

Well I followed lots of advice and installed the "Checkbox Validate" module, ran it, and then the issue was miraculously fixed.

Now when I create a test user, no verification email is sent out to the user, where it did before installing the module. The option to "Require e-mail verification when a visitor creates an account" is still checked in Administer > User Management > User Settings and it is still set in the DB.

Now I don't know how to roll back this module, I can't verify users and have no idea how to fix?

Any thoughts?

TIA

UPDATE - I went through and disabled the module and tried to register, which didn't issue the email. I thought it was this module which had caused this, but on further testing, I had applied an action which redirects the user to a "thanks for registering" page after registration. Only the trigger seems to have over-ridden the default action of sending the registration email. So in actual fact this was entirely my fault for enabling an action and a trigger, and nothing to do with this module.

Sorry [embarassed smile]

sun’s picture

Version: 6.16 » 7.x-dev
Category: support » bug

This issue is about Drupal 7 core, not about a contributed module.

See #259292: Required radios/checkboxes are not validated (D6) for D6. (although your issue doesn't sound related to Drupal core at all)

tim.plunkett’s picture

acbramley’s picture

Tried applying patch in #112 to 7.8 with no luck. Can a patch be rolled against it please?

mgifford’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +form validation

Sorry @Zombienaute but patches are rolled against HEAD, which is Drupal 8 in git and not a 7.x release. @Sun built that patch in November 13, 2009 and Drupal 7's changed a lot since then. It might be able to be back-ported, but first it needs to get into Core.

One could probably build a patch for this for D7 as a hack or look at extending the validation with a module.

acbramley’s picture

Makes sense, I might have a crack at doing it myself if I have the time.

mgifford’s picture

@acbramley - any chance you'll be joining us at the sprint this weekend?

Wanted to link to these related issues - #1493324: Inline form errors for accessibility and UX & #504962: Provide a compound form element with accessible labels