Required radios/checkboxes are not validated
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Annoying And Stupid Bugs That We Really Need To Just Fix Already |
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.
#1
I have this same problem. Has anyone come up with a solution?
#2
Until this gets fixed, my quick solution (and sucky) solution was to change radios/checkboxes to select fields.
#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.
#4
im having the same problem, will this bug be fixed soon ? (next some weeks)
#5
Subscribing... Any updates yet?
#6
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 ...
#7
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
#8
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
#9
this code changes affect only for the checkbox and not the radio box
#10
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?
#11
subscribing
#12
subscribing
#13
subscribing
#14
changed title ;)
#15
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
#16
The same problem.
subscribing
Tested on Drupal 5.7 + CCK 1.6
#17
Made a simple patch.
Problem was that #value wasn't empty() after nothing was selected.
#18
Is this patch also for Drupal v6.2 ?
Thanks,
Izzy
#19
subscribing
#20
subscribing
#21
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'
#22
Is the same problem that affects Profile core module in drupal 6.2?
http://drupal.org/node/259292
#23
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.
#24
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
#25
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.
#26
See also #259292: unckecked checkbox set to "required" bypasses validation which I marked a duplicate of this bug.
#27
Wonderful, Robert, thank you :-)
#28
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
#29
Hi bjacob, I think the module should also work on Drupal 5, maybe .info file might need adjusting to install properly.
#30
#17 doesn't work for me either. This even effects simple forms with a required checkbox for me.
--Andrew
#31
Does anyone know if this been fixed in D7?
#32
-12 days, and this bug will be 1 year old :-)
:-(
#33
Then someone better get on reviewing the patch and providing results, and then marking it RTBC. :P
#34
Marked #224012: required fields don't work correctly as a duplicate of this issue.
#35
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
#36
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.
#37
I am not able to replicate this bug in 7.x. Setting #required => TRUE on radios and checkboxes properly returns a validation error.
#38
Thanks for checking sun!
#39
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: unckecked checkbox set to "required" bypasses validation 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 :)
#40
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.
#41
Here is the patch along with the test. The patch does two things;
This passes all tests currently.
#42
Just marked #259337: Mandatory Profile's field is not mandatory as a duplicate.
#43
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.
#44
The last submitted patch failed testing.
#45
I was just able to install HEAD with this patch applied, so flipping back to CNR.
#46
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.#47
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
<?phpreturn !empty($edit) ? $form['#return_value'] : 0;
?>
should be:
<?phpreturn !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)
#48
The last submitted patch failed testing.
#49
I can confirm the results of the testing bot. This still needs work.
#50
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:
<?phpif (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.
#51
The last submitted patch failed testing.
#52
Reroll.
#53
The last submitted patch failed testing.
#54
I can't reproduce the testing failures.
#55
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...
#56
#55 -> #25
#57
@ 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!
#58
Re-roll of the patch from #52.
#59
The last submitted patch failed testing.
#60
subscribing
#61
Let's try again.
#62
The last submitted patch failed testing.
#63
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.
#64
So how about re-rolling and testing the patch, rather than re-iterating how much this needs to be fixed? :)
#65
@seanr: also check http://drupal.org/project/checkbox_validate
#66
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?
#67
@ 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.
#68
subscribe
#69
subscribe
#70
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.
#71
subscribe
#72
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.
#73
Ack. This isn't a hard thing to do. Doesn't take 2-3 weeks to re-roll a patch.
Subscribe.
#74
subscribing
#75
here's a re-roll...
#76
#77
The last submitted patch failed testing.
#78
subscribing
#79
A lot of tests broken, patch needs re-roll
#80
Subscribing... I gotta stick with checkbox_validate module for the meantime.
#81
re-rolling this patch.
#82
Setting status for the testbot.
#83
The last submitted patch failed testing.
#84
try again
#85
The last submitted patch failed testing.
#86
Updated patch
#87
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.
#88
The last submitted patch failed testing.
#89
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.
#90
#91
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
#92
Yes, it was wrong. First core patch rookie.
Once again, this is just a backporting to D6 from #86
#93
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
#94
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.
#95
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?
#96
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.
#97
The last submitted patch failed testing.
#98
Bot appears screwed up because of #87. Should the patch from #86 be resubmitted?
#99
I think it will be clear when the patch is being reviewed.
#100
The patch reuses the integer 0 which already is unsupported as a checkboxes key. Good trick.
#101
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.
#102
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.
#103
patch attached
#104
I think I like this :)
#105
The last submitted patch failed testing.
#106
Think this brings this patch up to the CVS.
#107
The last submitted patch failed testing.
#108
Hmm.. Rolled that patch wrong (more coffee in me now). Hopefully that's the problem, but these are the 9 fails.
#109
not quite enough coffee to change status to needs review.
#110
The last submitted patch failed testing.
#111
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', '') ));
}
// }
#112
Re-rolled #103 against HEAD + cleaned up the code.