The #required flag has an interesting (errant) behavior when dealing with textareas. Take comments for example: Go to any Drupal site and post a comment (this issue will work also) with nothing but a carriage return in the description or comment body. Even though the field is required, a single return is considered enough content to slip past the 'Required' field. Stranger still, the content is trimmed before it is inserted into the database, resulting in a completely empty post even though the field is required.

I've been seeing this issue mostly with comments, we get 5-10 anonymous empty comments a day by bots. Would trimming the field value be a better solution than using empty()?

Index: /Users/nate/Sites/lullabot/v1/public_html/includes/form.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/form.inc,v
retrieving revision 1.174.2.3
diff -u -r1.174.2.3 form.inc
--- form.inc	29 Jan 2007 21:51:53 -0000	1.174.2.3
+++ form.inc	10 Feb 2007 05:24:34 -0000
@@ -526,10 +526,7 @@
   /* Validate the current input */
   if (!isset($elements['#validated']) || !$elements['#validated']) {
     if (isset($elements['#needs_validation'])) {
-      // An empty textfield returns '' so we use empty(). An empty checkbox
-      // and a textfield could return '0' and empty('0') returns TRUE so we
-      // need a special check for the '0' string.
-      if ($elements['#required'] && empty($elements['#value']) && $elements['#value'] !== '0') {
+      if ($elements['#required'] && strlen(trim($elements['#value'])) == 0) {
         form_error($elements, t('!name field is required.', array('!name' => $elements['#title'])));
       }

Same patch also attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Version: 5.x-dev » 6.x-dev
Status: Needs review » Needs work
FileSize
1013 bytes

This seems very reasonable to me, except this problem also exists in HEAD and should be fixed there first. Also, the patch complains about unexpectedly ending. Here's a re-roll.

Now the only kind of weird thing is if I for example put in [space][space] as the title of a node, It will kick back and tell me "Title is required", but the [space][space] is still there, so the cursor appears more in the middle of the box -- I have to delete the two spaces in order to add my title. This does not happen, however, when I hit [return] in a comment body; there it puts the cursor right at the beginning of the textarea, as expected (so somewhere along the line it trimmed the input). We should standardize this behaviour.

Also, where did you see these things getting trimmed before they went in the db? We should remove that code if it's duplicate.

quicksketch’s picture

Thanks webchick. Not sure where things were getting trimmed before they went into the DB. I checked for string and regex replacements but couldn't find why this was taking place. Fortunately this patch (after you fixed it) mostly takes care of that problem. Now entering only returns or spaces into a required field will form_set_error on that field and leave the spaces or returns in place.

I like this approach more than actually trimming the value and making it empty again. Leaving the spaces in there but still throwing an error is inherently telling the user "spaces aren't enough for a title, dummy". Whereas if we trim the title back to an empty string then throw the error, the user will think "I put spaces in there didn't I?", thinking that might be enough. So in general we're avoiding modifying the user's input and instead just telling them it's not enough.

Unfortunately, in textareas if you enter a series of returns, every time you preview the post it looses another return. I'd like to figure that out before committing this patch.

Pancho’s picture

Status: Needs work » Needs review
FileSize
967 bytes

Here is a rerolled patch. Now we need to figure out what seems to happen with returns in textareas (see #2)... I need to figure out how to test this, as CCK is broken right now.

Pancho’s picture

Category: task » bug

Required textareas are a rare thing in core. Forum posts with their required body are a good example to test.
If I require 10 (or something) words for any other content type, I'm testing a different validation than the required field.

Results of the testing: The "losing a return" problem is still present, but it is obviously not connected to this patch. It also arises, when a completely different validation routine throws an error or the form is finally saved to db. I also experienced this bug without this patch being applied. Therefore I filed a new ticket #216588.

As the bug we hit is not connected to this patch, I think the patch in #3 is rtbc.
I do consider this a usability bug, for Title fields it is even critical.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

code looks good, though a code comment might be nice.

Works as advertised.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Hm, added some short docs and committed with that.

      // We only check for trimmed string length being zero. 0 might be
      // a possible value with some field types, such as radio buttons, so
      // empty() is not usable here.
webernet’s picture

Status: Fixed » Active

During Install:

notice: Array to string conversion in /includes/form.inc on line 670.

Gábor Hojtsy’s picture

Status: Active » Needs work

Doh, rolled this back. For reference, this was the patch I committed originally: http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/form.inc?r1=1.264...

RobLoach’s picture

Status: Needs work » Needs review
FileSize
836 bytes

Will it get the desired effect if we run a is_string on it? Might break something else, but worth attempting.

pwolanin’s picture

Status: Needs review » Needs work

gak, sorry about that - was focused on testing text fields. empty() does make sense for array elements, but causes its own problems.

how about something more like:

if ($elements['#required'] && (!count($elements['#value']) || (is_string($elements['#value']) && strlen(trim($elements['#value'])) == 0))) {

for any string (even an empty string) count() returns 1,

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

That made no sense. This will check if the value is actually a string, and then check if it's an empty string. This might give the desired result. Testing is definitely needed.

RobLoach’s picture

Status: Needs review » Needs work

Nope...

pwolanin’s picture

I don't know what webchick was posting, but I crossposted with both above.

@Rob Loach - I don't see that your code properly handles arrays - for example checkboxes that are required.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

This is a bit better....

RobLoach’s picture

Status: Needs review » Needs work

It seems like we should provide a switch statement to handle all required use cases (string, arrays, integers?).

Pancho’s picture

Rob Loach: Guess that's a good idea, as this is more transparent and thus also prevents future errors.

Pancho’s picture

Something like this would be cool:

switch ($elements['#datatype']) {
  case 'string':
    $empty = strlen(trim($elements['#value'])) == 0;
    break;
  case 'array':
    $empty = empty($elements['#value']);
    break;
  default:             // case 'integer':
    $empty = isset($elements['#value']);
}
if ($empty) {
  form_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));
}

but we don't have something like $elements['#datatype']. Good ideas?

pwolanin’s picture

see my comments above in #11- this last patch code is wrong since it fails to check arrays that may be empty

pwolanin’s picture

Status: Needs work » Needs review
FileSize
887 bytes

@Gabor - the rollback does not seem to have worked.

attached patch per the code in #11, seems to work for both checkboxes and textfields

Pancho’s picture

FileSize
1.43 KB

#20 should work, but we better check twice...
I just split the if-expression in two, as it was hard to grasp. Also I updated documentation.

pwolanin’s picture

@Gabor - or - perhaps you rolled-back on 6.x and not HEAD?

pwolanin’s picture

@Panch - your docs are not correct: "Count() checks if the item (string, array, integer) contains anything."

Patches attached for HEAD and DRUPAL-6

Pancho’s picture

Another suggestion for the docs:

// Validate required form items:
// Count() discards empty arrays and NULL values but not empty strings.
// Strings are checked not to be empty after being trimmed.

Also, I still prefer separate if-clauses for if ($elements['#required']) and the actual validation (see patch #21).

Gábor Hojtsy’s picture

Yes, I rolled back on 6.x, I am not entitled to commit anything to HEAD. Once we figure out how to solve this, it needs to be ported to HEAD too (or if we don't figure out anything, HEAD needs to roll back the patch as well).

pwolanin’s picture

@Gabor - I think the code in #23 works well, so it's just a question of the code comments.

RobLoach’s picture

Priority: Normal » Critical
FileSize
1.47 KB

This has to be fixed before the release of Drupal 6.0 as all these warnings when submitting forms would drive most users away. So, I'm setting this to critical.

I've tested the code from #23 in both a checkbox and textfield situation and both seemed to work as desired. The textfield where I passed an empty string was denied, and the checkboxes that I didn't check a required value were denied. I tested this in admin/settings/blogapi and admin/settings/site-information.

In my opinion, the code comments from #23 are fine, but this might seem more helpful for someone reading through the code (see patch)....

Pancho’s picture

Which warnings? The erroneous patch has been rolled back, so we are in the previous state, at least in the D6 branch.
Demoting this to normal. If I misunderstood you, feel free to make this critical again.
Even if not critical, we should get this committed soon, both in the D6 branch and in HEAD. But no rushing in - this time it should be 100%.

Pancho’s picture

Priority: Critical » Normal
webernet’s picture

Did a quick test of #23, and it doesn't appear to break anything.

Pancho’s picture

Status: Needs review » Reviewed & tested by the community

#23 and #27 is essentially the same code. There are two differences:

  1. #27 is more verbose in separating out the "if ($elements['#required'])" check, which I personally prefer.
  2. A different wording of the documentation.

This is nothing we can decide. Therefore it would be great if Gábor or Dries would take the decision and commit either of the two patches.
I mark this RTBC, as this is all we can do.

chx’s picture

I do *not* like the verbosity in #27. Thats code bloat in my eyes.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.31 KB
Gábor Hojtsy’s picture

Why do we only need to check for arrays and strings? Can someone shed a light on this?

pwolanin’s picture

@Gabor - data coming out of FAPI is always a string or an array. count(NULL) == 0, so that works. If you run trim() on a number or boolean, it's converted to a string, so that works too.

if someone really wants to return an object from a form(?!), they could implement the "countable" SPL interface. "This extension is available and compiled by default in PHP 5.":

http://us.php.net/manual/en/function.count.php

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

pwolanin: thanks for the explanation. Committed #33 to Drupal 6. Still needs to be committed to 7.x.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs review

So the comment could be clarified, maybe like:

+ // If the field is required, empty() cannot be used to check since
+ // the string '0' may be valid. count() gives the number of
+ // elements in an array or countable object, 0 for NULL, or 1.
+ // trim() removes white space, and converts to string.

pwolanin’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

oops- cross posted

Senpai’s picture

Status: Reviewed & tested by the community » Needs work

The error in #7 is still present during the installation of head. FYI

notice: Array to string conversion in /includes/form.inc on line 670.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

@Senpai - the bad patch still needs to be rolled back in HEAD and the good patch applied. This is only fixed in 6.x

pwolanin’s picture

here's a version of the patch from #33 that applies to HEAD.

RobLoach’s picture

Looks good.

chx’s picture

Until this is committed , HEAD will display a notice on install.

pwolanin’s picture

bump - RTBC and causing errors in HEAD

pwolanin’s picture

bump again...

Senpai’s picture

Bump a third time...

vladimir.dolgopolov’s picture

Here is the test for the issue.
Before #41 there were 31 passes, 2 fails and 2 exceptions.
After #41 there were 33 passes, 0 fails and 0 exceptions.


class TestCase117748 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[117748] Trim required fields on validate'),
      'desc' => t('The #required flag has an interesting (errant) behavior when dealing with textareas. Take comments for example: Go to any Drupal site and post a comment (this issue will work also) with nothing but a carriage return in the description or comment body. Even though the field is required, a single return is considered enough content to slip past the \'Required\' field. Stranger still, the content is trimmed before it is inserted into the database, resulting in a completely empty post even though the field is required. /quicksketch/'),
      'group' => t('Drupal 7 Tests'),
    );
  }
  
  // Check several empty values on standard forms' elements.
  // If the form field found in form_get_errors() then the test pass.
  function testIssue() {

    // Sets of empty strings and arrays
    $empty_strings = array("", "\n", " ", "\t", " \n\t ", "\n\n\n\n\n");
    $empty_arrays = array(array());

    $elements['textfield']['element'] = array('#title' => $this->randomName(), '#type' => 'textfield', '#required' => TRUE);
    $elements['textfield']['empty_values'] = $empty_strings;

    $elements['password']['element'] = array('#title' => $this->randomName(), '#type' => 'password', '#required' => TRUE);
    $elements['password']['empty_values'] = $empty_strings;

    $elements['password_confirm']['element'] = array('#title' => $this->randomName(), '#type' => 'password_confirm', '#required' => TRUE);
    $elements['password_confirm']['empty_values'] = $empty_strings;

    $elements['textarea']['element'] = array('#title' => $this->randomName(), '#type' => 'textarea', '#required' => TRUE);
    $elements['textarea']['empty_values'] = $empty_strings;

    $elements['radios']['element'] = array('#title' => $this->randomName(), '#type' => 'radios', '#required' => TRUE, '#options' => array($this->randomName(), $this->randomName(), $this->randomName()));
    $elements['radios']['empty_values'] = $empty_arrays;

    $elements['checkboxes']['element'] = array('#title' => $this->randomName(), '#type' => 'checkboxes', '#required' => TRUE,'#options' => array($this->randomName(), $this->randomName(), $this->randomName()));
    $elements['checkboxes']['empty_values'] = $empty_arrays;

    $elements['select']['element'] = array('#title' => $this->randomName(), '#type' => 'select', '#required' => TRUE, '#options' => array($this->randomName(), $this->randomName(), $this->randomName()));
    $elements['select']['empty_values'] = $empty_strings;

    $elements['file']['element'] = array('#title' => $this->randomName(), '#type' => 'file', '#required' => TRUE);
    $elements['file']['empty_values'] = $empty_strings;

    // Go through all the elements and all the empty values for them
    foreach ($elements as $type => $data) {
      $i = 1;
      foreach ($data['empty_values'] as $empty) {
        $form_id = $this->randomName();
        $form = $form_state = array();
        $form['op'] = array('#type' => 'submit', '#value' => t('Submit'));
        $element = $data['element']['#title'];
        $form[$element] = $data['element'];
        $form_state['values'][$element] = $empty;
        $form['#post'] = $form_state['values'];
        $form['#post']['form_id'] = $form_id;
        drupal_prepare_form($form_id, $form, $form_state);
        drupal_process_form($form_id, $form, $form_state);
        $errors = form_get_errors();
        $this->assertTrue(isset($errors[$element]), "Check empty(".$i++.") '$type' field '$element'");
      }
    }
    // Avoid output drupal warnings and messages into assert messages
    drupal_get_messages();
  }

}

catch’s picture

Title: Trim required fields on validate » admin/user/user/create is broken
Priority: Normal » Critical

http://drupal.org/node/232763 was duplicate.

Also this is blocking progress on http://drupal.org/node/228594 since admin/user/user/create is broken

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I'm not a big fan of this patch. I think it's clumsy. Furthermore, I think we should be using drupal_strlen() instead of strlen(). Please verify. Thanks.

pwolanin’s picture

@Dries - strlen() should be fine (and much faster) since we just want a boolean answer of "characters or no characters".

Do you have suggestions? I think it's less clumsy than what we had before.

chx’s picture

Clumsy...? Call PHP clumsy, then.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

@Dries - this patch is already in D6, and fixes a critical bug. If you have suggestions for better code for D6 and or D7, can they be addressed after HEAD works again?

Dries’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

OK, I've committed this to CVS HEAD to make HEAD work again. I'm marking this 'code needs work' as the test needs to be extracted into a simpletest test.

pwolanin’s picture

@Dries - thanks for getting the fix in, but I'm confused about what tests you are referring to?

catch’s picture

pwolanin - did you see the test in #47

pwolanin’s picture

Status: Needs work » Fixed

oh, right, THAT test.

Making minor changes to the test and posting as a patch: http://drupal.org/node/231302

pwolanin’s picture

Title: admin/user/user/create is broken » Trim required fields on validate
Version: 7.x-dev » 5.x-dev
Status: Fixed » Patch (to be ported)

how about 5.x?

igorik’s picture

Priority: Normal » Critical

Yes, there is a little missunderstood which patch from all of them here is ok to apply to Drupal 5,
is it #23 - http://drupal.org/node/117748#comment-717485 ?

I have critical problem with empty nodes atomaticaly created when some user normal goes through site, he add no content...
so mabe this can be solution for it (http://drupal.org/node/236986)

thanks
Igorik
http://www.somvprahe.sk

pwolanin’s picture

none of these patches work for 5.x, hence the status "to be ported"

pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.35 KB

here's an UNTESTED patch for 5.x

drumm’s picture

Priority: Critical » Normal

I do not think this is critical, it is only an edge case.

@igorik, maybe you should check with your problem users or see if a contributed module or somesuch is causing the problem.

pwolanin’s picture

@drumm - right, sorry, it was critical for HEAD since some forms were failing there due to the bad 1st patch that was committed.

janusman’s picture

subscribing

treksler’s picture

Status: Needs review » Reviewed & tested by the community

Applied #60 to my Drupal 5.8 installation.

Doesn't appear to break anything and looks it is a very welcome bugfix that allows me to remove all the duplicate validation code in my module that is currently checking for empty values in required fields.

janusman’s picture

Ditto, it works here too =)

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

threexk’s picture

Version: 5.x-dev » 5.10

This change caused many of my required CCK radio button fields to give the following error when left unchosen at submission:
"An illegal choice has been detected. Please contact the site administrator."

The code-level problem was these fields had value Boolean false, and count() in the new line of code returns 1 for that. Oddly I was able to fix it on a per-field basis by editing and resaving the field in the CCK Manage Fields pages. This made the fields take on the empty string instead of Boolean false.