In the process of investigating http://drupal.org/node/35467, I found that blank required form fields are not detected anymore.
I think the problem is in form.inc:
@@ -138,7 +138,7 @@ function _form_validate($elements) {
/* Validate the current input */
if (!$elements['#validated'] && $elements['#input']) {
* if ($elements['#required'] && !isset($elements['#value'])) {
form_error($elements, t('%name field is required', array('%name' => $elements['#title'])));
}
if ($elements['#valid']) {
The !isset does not work for a field that is blank. I tried replacing it with empty(), which works most of the time but empty() also returns TRUE for a string "0" and a integer 0.
I created a new function drupal_empty() to replace the !isset
function drupal_empty($value = NULL) {
if (isset($value)) {
if (empty($value) && ($value !== 0) && ($value !== '0'))
return TRUE;
else
return FALSE;
}
else
return TRUE;
}
(it's not optimised or anything, just copied from the php help)
And that seems to work for the required form fields and '0' string fields are handled properly.
Is this the best course of action? Are there other places in the code which don't work quite right because of isset()/empty() which could also use a function like this?
Comment | File | Size | Author |
---|---|---|---|
#7 | form-inc-validate-fix_1.patch | 897 bytes | asimmonds |
#5 | form-inc-validate-fix_0.patch | 895 bytes | chx |
#1 | form-inc-validate-fix.patch | 932 bytes | asimmonds |
Comments
Comment #1
asimmonds CreditAttribution: asimmonds commentedI've in-lined these extra conditions into _form_validate()
Comment #2
chx CreditAttribution: chx commentedIf you thought that 'POST values are strings, why === 0?' then think of empty checkboxes.
That said, this applies and is a must.
Comment #3
Dries CreditAttribution: Dries commentedAren't we doing the same check
($elements['#value'] !== 0)
twice?That doesn't look right to me ...
Comment #4
chx CreditAttribution: chx commentedwe are checking first for 0 and then for '0'
Comment #5
chx CreditAttribution: chx commentedDries is somewhat right, the '0' check is now not needed, that would kill a literal 0 in a textfield for example. Empty checkbox is 0 not '0'.
Comment #6
asimmonds CreditAttribution: asimmonds commentedWith patch #5, a textfield with a literal '0' in it is still seen as required.
Changing it to:
allows the '0' in a textfield as well as still detecting a unchecked set of checkboxes.
(Sorry, can't roll a patch at the moment)
Comment #7
asimmonds CreditAttribution: asimmonds commentedAnd here's the patch with that change.
Tested against a form with:
single checkbox, radios set, checkboxes set, textfield, select, multiple-select, weight
all controls with #required => TRUE (I wanted to see if did anything weird with the other controls)
textfield errored on blank, but not '0', 0.0, or -0
checkboxes set errored on all unchecked, but OK on at least one checkbox checked
single checkbox errored on unchecked, OK on checked (but why have a required single checkbox?)
all other controls seemed to behave properly
Comment #8
Mad Maks CreditAttribution: Mad Maks commentedthis patch works for me, thanks
Comment #9
chx CreditAttribution: chx commentedLooks good, still applies.
Comment #10
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #11
(not verified) CreditAttribution: commented