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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

asimmonds’s picture

Status: Active » Needs review
FileSize
932 bytes

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

chx’s picture

Assigned: Unassigned » chx
Priority: Normal » Critical
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.

Dries’s picture

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

chx’s picture

Status: Needs work » Reviewed & tested by the community

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

chx’s picture

FileSize
895 bytes

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

asimmonds’s picture

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

Changing it to:

    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)

asimmonds’s picture

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

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

Mad Maks’s picture

this patch works for me, thanks

chx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, still applies.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)