Posted by TyraelTLK on May 16, 2008 at 2:20pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
To reproduce:
1) Create a new field in the profile: a "checkbox" with "The user must enter a value." "Visible in user registration form."
2) Register a user, without filling in the checkbox.
If reproduced properly despite the required field not being filled in the user is registered.
Reletad to http://drupal.org/node/179932 ?
Comments
#1
I noticed the problem as well.
I was using a theming function to add a required checkbox to the 'user_register' form, it wasn't marked as required.
Then I used hook_form_alter to do the same. Still, the checkbox was not marked as required.
So I added a checkbox through the profile.module, using "The user must enter a value." and "Visible in user registration form."
Still the checkbox is not marked as required.
When using the profile.module and adding other types of fields, such as 'text field', with "The user must enter a value." and "Visible in user registration form.", fields are marked as required.
This is a problem with checkbox ONLY, as far as I can tell.
Since this problem can be reproduced beyond the profile.module, I am changing the 'component' of this issue to 'forms system'.
Since this seems to affect the Forms API generally, I am marking this as 'critical'.
#2
Maybe this isn't critical...
#3
I just made different tests... adding required checkbox in different forms.
Doesn't work for any form.
I checked the Drupal Form API, there's no trick about it, setting the '#required' property to TRUE (or 1) *should* work.
#4
The problem is in form.inc I think.
Line 666, number of the beast! (evil code)
<?phpif (isset($elements['#needs_validation'])) {
// Make sure a value is passed when the field is required.
// A simple call to empty() will not cut it here as some fields, like
// checkboxes, can return a valid value of '0'. Instead, check the
// length if it's a string, and the item count if it's an array.
if ($elements['#required'] && (!count($elements['#value']) || (is_string($elements['#value']) && strlen(trim($elements['#value'])) == 0))) {
form_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));
}
?>
#5
I am not familiar enough with the form API at this point to come up with proper conditions.
#6
This is a serious problem.
You can't launch a commercial site without
[x] I agree.
Or
[x] I CERTIFY THAT I AM OVER THE AGE OF 18 YEARS OLD.
#7
#8
A special case could be made for checkboxes in _form_validate, for the rare REQUIRED checkbox, I think.
// Make sure a value is passed when the field is required.// A simple call to empty() will not cut it here as some fields, like
// checkboxes, can return a valid value of '0'. Instead, check the
// length if it's a string, and the item count if it's an array.
if ($elements['#required'] && (!count($elements['#value']) || (is_string($elements['#value']) && strlen(trim($elements['#value'])) == 0))) {
form_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));
}
one might add another clause to the IF, something like this pseudo code:
|| (TYPE == 'checkbox' && ( VALUE == 0 || VALUE == "0" || VALUE == false || strtoupper(VALUE) == 'FALSE'))
#9
I created a module to 'circumvent' the bug for the registeration form: http://drupal.org/project/terms_of_use
I am using the property '#element_validate' on the checkbox, hence I do my own validation through a function of my own.
#10
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 code to their modules to fix 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.
#11
Either this or #179932: Required radios/checkboxes are not validated is a duplicate. I've marked this one a dupe since it is newer, although the other was originally applied against another project, which is probably why it was missed.
#12
subscribe ..
#13
not a duplicate of #179932: Required radios/checkboxes are not validated anymore as that bug fixed only in 7.x
#14
Subscribe. Switched to 6.x dev (patches should be made against HEAD).
#15
Subscribing.
#16
#17
subscribing
#18
syncing tag / title with D7. (sun.core asked for a separate D6 issue).
#19
subscribing
#20
subscribing
#21
subscribing
#22
I found the problem -- the conditional for validating required fields was only testing for zero-length values or values that weren't set. In the case of an unchecked checkbox, the value will be set to the integer zero, so I added handling for that specific case. Give it a shot!
#23
As this is the same issue than in http://drupal.org/node/179932, I am backporting the D7 solution for this problem:
required checkboxes are not validated and there is no required warning in the title.
For the validation issue, this is a back porting from D7.
For the required warning, obviously the Form API in D6 is different from D7, but this patch add exactly the same behavior than in D7 and for the rest of required form elements.
In theme_checkbox(), we add the same warning elements that are added in theme_form_element() function for the other required form elements.
It adds as well the $t=get_t() used in theme_form_element() for the rest of the form elements and as in D7.
It works great for CCK required checkboxes, and forms using this elements, as in legal module (with this core patch the checkbox_validate module wouldn't be necessary any more)
#24
subscribing...
#25
#23 did the trick, thanks!
Is there a chance of this getting committed into the Drupal 6 version?
#26
#27
The patch makes it so I no longer need to use the contrib module http://drupal.org/project/checkbox_validate
Thanks jcmarco for the D6 patch!
#28
Due to the follow-up issues/problems identified with the patch that went into D7 (#179932: Required radios/checkboxes are not validated), I'm not sure whether it is a wise idea to apply this change to D6.
I'd actually opt for a won't fix here. People can use checkbox_validate module for D6 and below. For D7, you won't, but we still have to figure out some major problems related to required/disabled checkboxes.
#29
@sun: to which issues are you referring to? (issue numbers or post links please?)
If this (latest patch in #23) raises issues, but the Checkbox validate module on the other hand fixes them, why not merge this module in d6 core? At least in this case see how the module gets things done and mimic it for a final solution in core.
#30
@sun - the problem is, this is a pretty serious bug in D6, and should probably be fixed, since the settings page says "make this required," and it doesn't become required. So I'd support seeing this patch get in :-/
#31
Problem space being that checkbox_validate only fixes required checkboxes. D7 additionally hit the territory of disabled checkboxes, not covered/supported by checkbox_validate, and which may or may not work in D6 currently. Note that there are a couple of possible combinations to support, i.e., required only, disabled only, required and disabled (but checked), and normal (nothing).
While something has been committed to D7 for #179932: Required radios/checkboxes are not validated, that issue basically is still open and still major. One of the other D7 issues that aforementioned commit "broke" is #654796: Identify fix bugs with checkbox element, but actually I'm not sure whether there are not some more interrelated issues. I definitely have to spare some time to clean up this mess.
#32
@geerlingguy: Jeff, I am totally with you! This definitely needs to be fixed in core.
@sun: Thanx for taking the time to reply, Daniel. I just didn't get why it was closed, but I realized it was simply because didn't read through the whole issue. So, I guess we need to wait on the second issue and once we have a solid fix for d7 then backport to d6(?).
#33
The last submitted patch, form.inc_.patch, failed testing.
#34
bad bot! This is a D6 patch.
#35
subscribing (too bad, also Checkbox Validate fails to correct this..)
#36
Isn't there also the problem that form_error() won't work with either checkboxes nor select fields? I've implemented my own validation on checkboxes and selects and get no effect whatsoever, Drupal just moves on to form submission and doesn't go back on a form_error on either of those fields.
#37
It looks as if form_error() fails with checkboxes and selects is because they don't come with a '#parents' element in the field definition. All other field types get that assigned automatically.
#38
The last submitted patch, form.inc_.patch, was successfully tested.
#39
Hi,
there is a second problem with the checkbox-validation.
If a CCK-Checkbox-field is required and the user dont fill out this field and he will save the add-form then the checkbox-field will not get an DIV-error-classes.
If it is a normal text-field or a radio-button-field the error-class will set.
Examples for fields after saving the add-form (without fill out the field)
normal text-field:
<input type="text" class="form-text required error" id="edit-field-1-adresse-0-city" name="field_1_adresse[0][city]">text-field (radio button):
<input type="radio" class="form-radio error" value="Kosten angeben" name="field_1_kosten1[value]" id="edit-field-1-kosten1-value-Kosten-angeben">text-field (checkbox) (no error-class / no required-class will set):
<input type="checkbox" class="form-checkbox" value="girls" id="edit-field-1-test-value-girls" name="field_1_test[value][girls]">In form.inc in line 2246 the code is ...
<?phpfunction _form_set_class(&$element, $class = array()) {
if ($element['#required']) {
$class[] = 'required';
}
if (form_get_error($element)) {
$class[] = 'error'; // <--- there
}
if (isset($element['#attributes']['class'])) {
$class[] = $element['#attributes']['class'];
}
$element['#attributes']['class'] = implode(' ', $class);
}
?>
but this works not for checkboxes ......
Has anyone an idea to fix this problem?
Sorry for my bad english, i hope you understand the problem.
Best Regards Matthias
lirix
#40
patch from #23 working for me too, and I'm hard to work with :)
thanks
#41
In the validation function you can add after form_error():
<?php$element['#attributes']['class'][] = 'error';
?>
With the system css this will attempt to add a 2px red border to the checkbox. However, not all browser and OS combinations allow checkboxes to be styled. The workaround for this would be to use a background image for the checkbox.
#42
@JStarcher
Thank you for your answer but in what line this code must in?
In this (line 692 in form.inc)?
<?phpform_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));
?>
Can you help me please? Im am not a coder-geek!
Regards Matthias
#43
@JStarcher
I add the code!
<?phpform_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));
$elements['#attributes']['class'][] = 'error';
?>
When i output now the required $elements (and i not fill out this) with Devel dsm($elements); then the #attributes-Array has the correct error-class-entry now. But in the html-source-text the required checkbox has anyhow no error-class (when i dont fill out the checkbox and then will save the node-add-form.) I get the message "Field xxx is requiered" but no error-class is available!
I dont understand this! Has you an idea?
Sorry for my bad english!
Edit:
This code will not work. Now - when you dont fill out a normal required text-field and then save the add-form it will get an error
Fatal error: [] operator not supported for strings ...
???
#44
For Drupal 6:
With this code (in your template.php or as own module) the error class will set to the label from checkbox when checkbox is required but the user dont fill out it and he save the form.
<?php
function YOURTHEME_form_element($element, $value) {
// This is also used in the installer, pre-database setup.
$t = get_t();
$output = '<div class="form-item"';
if (!empty($element['#id'])) {
$output .= ' id="' . $element['#id'] . '-wrapper"';
}
$output .= ">\n";
if (form_get_error($element)) {
$required = !empty($element['#required']) ? '<span class="form-required error" title="' . $t('This field is required.') . '">*</span>' : '';
}
else
{
$required = !empty($element['#required']) ? '<span class="form-required" title="' . $t('This field is required.') . '">*</span>' : '';
}
if (!empty($element['#title'])) {
$title = $element['#title'];
if (!empty($element['#id'])) {
$output .= ' <label for="' . $element['#id'] . '">' . $t('!title: !required', array('!title' => filter_xss_admin($title), '!required' => $required)) . "</label>\n";
}
else {
$output .= ' <label>' . $t('!title: !required', array('!title' => filter_xss_admin($title), '!required' => $required)) . "</label>\n";
}
}
$output .= " $value\n";
if (!empty($element['#description'])) {
$output .= ' <div class="description">' . $element['#description'] . "</div>\n";
}
$output .= "</div>\n";
return $output;
}
?>
The error class is (for example) important when you use "Vertical Tabs". In Vertical Tabs you get a lot of fields in different Tabs and when the user dont fill out the required-Checkbox in Tab Nr. 6 (for example) - then will show (indeed) an Error-Message (Field xyz is required) but the correct Tab will not show the red-Color (because the error class is not availiable in checkbox) and the user dont know in what Tabs the required field is. Therefore he must control all Tabs to find the required field.
Code in top fix this problem, checkbox-label get an error-class and the correct Tab (where field is in) will show in red.
Sorry for my bad english.
Regards Matthias
#45
subscribing.
#46
Patch works for me with a custom form. Not tried it with CCK fields.
#47
@Berliner Thanks for your code.
I put your code from #44 in template.php in my theme folder and it works for single check box and multiple radio button. But not for multiple checkbox. .
I tried This code also and again works only for single check box and multiple radio button but not multi checkbox.
Any idea?