Trim required fields on validate
quicksketch - February 10, 2007 - 05:36
| Project: | Drupal |
| Version: | 5.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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.
| Attachment | Size |
|---|---|
| required_trim.patch | 1004 bytes |
#1
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.
#2
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.
#3
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.
#4
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.
#5
code looks good, though a code comment might be nice.
Works as advertised.
#6
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.
#7
During Install:
notice: Array to string conversion in /includes/form.inc on line 670.
#8
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...
#9
Will it get the desired effect if we run a is_string on it? Might break something else, but worth attempting.
#11
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,
#12
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.
#13
Nope...
#14
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.
#15
This is a bit better....
#16
It seems like we should provide a switch statement to handle all required use cases (string, arrays, integers?).
#17
Rob Loach: Guess that's a good idea, as this is more transparent and thus also prevents future errors.
#18
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?
#19
see my comments above in #11- this last patch code is wrong since it fails to check arrays that may be empty
#20
@Gabor - the rollback does not seem to have worked.
attached patch per the code in #11, seems to work for both checkboxes and textfields
#21
#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.
#22
@Gabor - or - perhaps you rolled-back on 6.x and not HEAD?
#23
@Panch - your docs are not correct: "Count() checks if the item (string, array, integer) contains anything."
Patches attached for HEAD and DRUPAL-6
#24
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).#25
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).
#26
@Gabor - I think the code in #23 works well, so it's just a question of the code comments.
#27
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)....
#28
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%.
#29
#30
Did a quick test of #23, and it doesn't appear to break anything.
#31
#23 and #27 is essentially the same code. There are two differences:
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.
#32
I do *not* like the verbosity in #27. Thats code bloat in my eyes.
#33
#34
Why do we only need to check for arrays and strings? Can someone shed a light on this?
#35
@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
#36
pwolanin: thanks for the explanation. Committed #33 to Drupal 6. Still needs to be committed to 7.x.
#37
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.
#38
oops- cross posted
#39
The error in #7 is still present during the installation of head. FYI
#40
@Senpai - the bad patch still needs to be rolled back in HEAD and the good patch applied. This is only fixed in 6.x
#41
here's a version of the patch from #33 that applies to HEAD.
#42
Looks good.
#43
Until this is committed , HEAD will display a notice on install.
#44
bump - RTBC and causing errors in HEAD
#45
bump again...
#46
Bump a third time...
#47
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.
<?php
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();
}
}
?>
#48
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
#49
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.
#50
@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.
#51
Clumsy...? Call PHP clumsy, then.
#52
@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?
#53
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.
#54
@Dries - thanks for getting the fix in, but I'm confused about what tests you are referring to?
#55
pwolanin - did you see the test in #47
#56
oh, right, THAT test.
Making minor changes to the test and posting as a patch: http://drupal.org/node/231302
#57
how about 5.x?
#58
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
#59
none of these patches work for 5.x, hence the status "to be ported"
#60
here's an UNTESTED patch for 5.x
#61
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.
#62
@drumm - right, sorry, it was critical for HEAD since some forms were failing there due to the bad 1st patch that was committed.
#63
subscribing
#64
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.
#65
Ditto, it works here too =)
#66
Committed to 5.x.
#67
Automatically closed -- issue fixed for two weeks with no activity.