Trim required fields on validate

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.

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.

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.

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

Works as advertised.

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.

During Install:

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

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

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

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,

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.

Nope...

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.

This is a bit better....

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

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

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?

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

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

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

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

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

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

Patches attached for HEAD and DRUPAL-6

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

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

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

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

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

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

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

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

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

@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

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

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.

oops- cross posted

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.

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

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

Looks good.

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

bump - RTBC and causing errors in HEAD

bump again...

Bump a third time...

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();
  }

}

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

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.

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

Clumsy...? Call PHP clumsy, then.

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

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.

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

pwolanin - did you see the test in #47

oh, right, THAT test.

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

how about 5.x?

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

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

here's an UNTESTED patch for 5.x

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.

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

subscribing

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.

Ditto, it works here too =)

Committed to 5.x.

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.