Blank required form fields not detected

asimmonds - October 30, 2005 - 11:54
Project:Drupal
Version:x.y.z
Component:base system
Category:bug report
Priority:critical
Assigned:chx
Status:closed
Description

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

<?php
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?

#1

asimmonds - November 15, 2005 - 10:41
Status:active» needs review

I've in-lined these extra conditions into _form_validate()

AttachmentSize
form-inc-validate-fix.patch 932 bytes

#2

chx - November 15, 2005 - 13:44
Priority:normal» critical
Assigned to:Anonymous» chx
Status:needs review» reviewed & tested by the community

If you thought that 'POST values are strings, why === 0?' then think of empty checkboxes.

That said, this applies and is a must.

#3

Dries - November 15, 2005 - 20:36
Status:reviewed & tested by the community» needs work

Aren't we doing the same check ($elements['#value'] !== 0) twice?
That doesn't look right to me ...

#4

chx - November 15, 2005 - 20:57
Status:needs work» reviewed & tested by the community

we are checking first for 0 and then for '0'

#5

chx - November 15, 2005 - 21:02

Dries 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'.

AttachmentSize
form-inc-validate-fix_0.patch 895 bytes

#6

asimmonds - November 16, 2005 - 02:53

With patch #5, a textfield with a literal '0' in it is still seen as required.

Changing it to:

<?php
   
if ($elements['#required'] && empty($elements['#value']) && $elements['#value'] !== '0') {
?>

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)

#7

asimmonds - November 16, 2005 - 04:49
Status:reviewed & tested by the community» needs review

And 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

AttachmentSize
form-inc-validate-fix_1.patch 897 bytes

#8

Mad Maks - November 18, 2005 - 07:33

this patch works for me, thanks

#9

chx - November 21, 2005 - 14:17
Status:needs review» reviewed & tested by the community

Looks good, still applies.

#10

Dries - November 21, 2005 - 16:23
Status:reviewed & tested by the community» fixed

Committed to HEAD. Thanks.

#11

Anonymous - December 5, 2005 - 16:40
Status:fixed» closed
 
 

Drupal is a registered trademark of Dries Buytaert.