Add trim to all text field submissions

I also think that it would be more user-friendly to trim the email address before the validation. This patch trims the email address field before the validation.

tested.

1) there is a little problem: the validation works with the trimmed email, but the UNtrimmed email is saved in the DB (have a look at user/$uid/edit: the leading/trailing space is still there.
2) the problem exists in cvs: please give a patch for cvs first and it will be backported.
3) provide a patch from the Drupal root directory. see: http://drupal.org/patch

Strange, when you submit the user form the modifications to the $edit in the user_user function are not kept between calls.

  if ($type == 'validate' && $category == 'account') {
    echo "<br>VALIDATE<br>";
    var_dump($edit['mail']);
    $res =  _user_edit_validate(arg(1), $edit);
    var_dump($edit['mail']);
  }
  if ($type == 'submit' && $category == 'account') {
    echo "<br>SUBMIT<br>";
    var_dump($edit['mail']);
    return _user_edit_submit(arg(1), $edit);
  }

If I submit this with email address " person@example.com" the first dump shows the same. After the validation (with the trim patch applied) it's "person@example.com" (good) but once you come back in the 'submit' part it's again " person@example.com" (not good).

Shouldn't the modifications in the validate part go to the submit part? Or should the modifications applied in the validate part be applied again in the submit part? The latter seems somewhat strange to me.

Ok, one cannot modify the form data in the validate step (http://drupal.org/node/22218#node_hook_order). The doc is for node submits but I guess this is also true for user data submits.

So there is the 'prepare' and 'form' step left to make the change. The user_user function does not get a 'prepare' step, so the only possible place is 'form'. But when you make changes to the $edit array they are not reflected in the subsequent steps! When you make changes in the 'form' step, shouldn't the 'submit' step see those changes?

Still an issue.

As we now have form_state we can modify during the validate.

Patch provided to tirm the email address.

Carriage returns were stripped during the patch, but it works for me. However, should it be used for all email fields in Drupal or is the registration page a special case? I also noticed that the request new password form already ignores any surrounding spaces.

It should be checked, also, the email while installing and in the contact form

Verified issue
installed patch
now receive 404 errors on /user/password and /user/register

Verified issue
installed patch
now receive 404 errors on /user/password and /user/register

Have just applied this patch to the current cvs version of drupal and I can not replicate this issue.

Please roll back the patch and confirm that the issue goes away.

Also please try a fresh install of drupal 6.

This patch doesn't touch the menu system so there is no reason for a 404 to occur.

Yes, I think this is very confusing for final users. the system will reject the email address with an error message that is contradictory and confusing since there is nothing apparently wrong with the email address.

It does not apply only to the user registration form. There are many forms throughout Drupal where an email address is requested.

The patch trim the spaces off of the email address in all the forms that currently requires an email address. The email is modified over the $form_state variable (or a reference) such that the submit handler saves the trimmed version to the database.

Affected files are:
user.module
comment.module
contact.pages.inc
system.module
system.admin.inc
install.php

Take a look to the screen shots for details.

Please test, This is a bug that would be good to fix for the next release of Drupal.

I agree that we need to patch all areas in drupal where the email is an issue. If we don't I think the situation could end up being even more confusing for users.

I suppose the question now is whether or not the the version number for this issue should be set to 7.x-dev. This approach will allow a better discussion to take place about the issue.

I installed dropcube's patch to Drupal 6 RC2 and I verified the fix on the following pages:

My account
Site information
Configure site
Add user
Contact Form
Comments

This should go against HEAD first, before being backported.

Also, the patch introduces trailing white-space (which is kind of ironic, I think :)) on line 1495 of user.module.

Rerolled against head.

I wonder if trimming should simply be part of the Form API; i.e an get applied to all fields?

@Dries -- that's an interesting point. I'm trying to think of a valid use case for not trimming text and I can't think of any. Any one else think of one?

The fact that we can't think of one it doesn't mean there can't be one. But apart that, I think trimming should be done on all fields.

If we trim on all fields, I think we might be able to remove quite a few trims. :-)

Works for me too. At least a couple of times I've looked for a node title direct in the database and not found it because of a leading space, or it's messed up a views alpha listing.

Let's try it at least. Changing issue scope.

I took at look at form.inc (which is still pretty much a black hole to me), but what about something like (from _form_validate):

<?php
  // Recurse through all children.
  foreach (element_children($elements) as $key) {
    if (is_string($elements[$key]) {
      $elements[$key] = rtrim(ltrim($elements[$key]));
    }
    if (isset($elements[$key]) && $elements[$key]) {
      _form_validate($elements[$key], $form_state);
    }
  }
?>

don't laugh too hard if it's totally off ;-)

@WorldFallz
Why not just a trim()? Anyway, a proper patch file is needed.

i didn't use trim, because there could be valid reasons for white space in the middle of a string depending on the input format and i didn't realize trim() only touches the beginning/end, lol.

And I didn't want to waste time on a patch that was completely off-base. If this is the correct way to handle it, i'll roll a proper patch (including removal of any existing trim()'s where necessary).

_form_validate and valdiation handlers aren't supposed to change an element's value. The correct place to perform the trim() is in function form_type_textfield_value().

One-line patch attached.

Makes a lot of sense, but let's add a little test for that.

Sure.

The doxygens are a little bit off. Also, I would feel more comfortable if we made a request to a real form on the child site instead of using FAPI internal functions for this.

Better spelling and docs. No time right now to write another test that uses the browser. If anyone wants to have a go, great! Otherwise, IMO the test as it stands is enough. We test that submitting forms via the browser works in many many places..

Thanks for taking this for a test drive -- I think it might work. One thing we should do is try to figure out if there are some trims() that can be removed from submit handlers now. Candidates:

deimos:drupal-cvs dries$ grep -r form_state . | grep "trim("
./modules/block/block.admin.inc:        'pages' => trim($form_state['values']['pages']),
./modules/block/block.admin.inc:        'pages' => trim($form_state['values']['pages']),
./modules/filter/filter.admin.inc:    $name = trim($form_state['values']['name']);
./modules/filter/filter.admin.inc:  $name = trim($form_state['values']['name']);
./modules/node/content_types.inc:  $type->type = trim($form_state['values']['type']);
./modules/node/content_types.inc:  $type->name = trim($form_state['values']['name']);
./modules/node/content_types.inc:  $old_type = trim($form_state['values']['old_type']);
./modules/node/content_types.inc:  $type->type = trim($form_state['values']['type']);
./modules/node/content_types.inc:  $type->name = trim($form_state['values']['name']);
./modules/node/content_types.inc:  $type->orig_type = trim($form_state['values']['orig_type']);
./modules/node/node.module:    if (trim($form_state['values']['teaser_js'])) {
./modules/node/node.module:      $body = trim($form_state['values']['teaser_js']) . "\r\n<!--break-->\r\n" . trim($form_state['values']['body']);
./modules/node/node.module:    form_set_value($form['basic']['inline']['processed_keys'], trim($keys), $form_state);
./modules/path/path.admin.inc:  $form_state['redirect'] = 'admin/build/path/list/' . trim($form_state['values']['filter']);
./modules/search/search.module:  $form_state['redirect'] = 'search/node/' . trim($form_state['values'][$form_id]);
./modules/search/search.pages.inc:  form_set_value($form['basic']['inline']['processed_keys'], trim($form_state['values']['keys']), $form_state);
./modules/system/system.admin.inc:  $ip = trim($form_state['values']['ip']);
./modules/system/system.admin.inc:  $ip = trim($form_state['values']['ip']);
./modules/update/update.settings.inc:    foreach (explode("\n", trim($form_state['values']['update_notify_emails'])) as $email) {
./modules/user/user.pages.inc:  $name = trim($form_state['values']['name']);

First try to remove all trim(). I don't know if passwords are affected by the trim patch, so I didn't remove those that were in password validations.

The Zend_Form components let's you specify filters on form fields:

class ExampleForm extends Zend_Form {
	public function init() {
		$this->addElement('text', 'description', array(
				'label' => 'Description',
				'filters' => array('StringTrim'),
		));
	}
}

So maybe the filter functionality can be added to the form API in stead of trimming all strings by default? This patch doesn't even allow people to not trim a text field...

Zend Framework also lets you apply filters to all the elements on a form which might also be an option for the Drupal form api.

It does allow not to trim a textfield by setting a custom #value_callback on the element.

Implementing multiple value filters is still a nice idea. This could maybe be achieved by making #value_callback an array and passing the value through all #value_calback functions. Let's keep that for a seperate issue, though.

I agree with Frando in #35 but it is something which can be left for a future patch. We could add a TODO to the code if desirable, but it shouldn't hold up this patch.

It looks like this patch has another patch embedded into it. Somehow that does not bother the test bot so we might be alright here. Given that the test bot is happy, I think it is safe to proceed with this patch.

This one should be better and incorporates Frando's patch too.

tic2000, the patch you submitted is malformed or broken.

Proper reroll attached. Quickly skimmed the places where trim() was removed now that it's default on textfield and it looked good. Removed the changes to search.module, though (they aren't directly coming from textfield value so we better leave the trim()s in).

Let's see what testbot says.

I've rerolled the patch for current head.

Bumping to 8.x (wow, that looks weird :x).

Also, I want to stress tic2000's concern in comment 33, that trimming all fields will affect passwords as well - which might have deliberate whitespace at the beginning or end.

Only feature requests and tasks can be assigned to D8 for now, because the Drupal 8 tree is not opened yet.

Fortunately, this is a feature request.

I think this is a fair assumption to make, but unconditionally applying trim() to all input=text field values without providing an opt-out is not a fair thing to do.

In other words: Let's turn this into a #value_trim property for textfields in system_element_info(), which defaults to TRUE.

Use-cases that re-use textfields for other applications can easily opt-out then.

Btw, this functionality is also interesting for #type textarea, though a default of FALSE might be more appropriate for them.

I have taken a stab at this. It needs tests still.

Thanks for reviving this, @aaron!

This topic came up very recently in a lot of config system conversions, since we're currently trimming values in entity storage controllers, too.

Personally, I think that whenever someone calls into an entity API directly, we should assume that developers know what they are doing. Contrary to that, user input supplied via forms can contain arbitrary leading and trailing white-space, and we only need to cater for that.

I like the new #trim property in #50. However, I'd almost move its processing as a generic facility into form_handle_input_element(), before any value callback is invoked; i.e., something along the lines of this:

if (!empty($element['#trim']) {
  $input = trim($input);
  // If the trimmed input is empty, ensure that #required and
  // other validation handlers are triggered accordingly.
  if ($input === '') {
    $input = NULL;
  }
}

What do you think?

ok

Sorry, my snippet was misleading/wrong. The following code checks for $input_exists instead of $input, so we need to force that to FALSE.

I've adjusted that, and additionally:

- Changed #type 'textarea' have a default #trim of TRUE.

- Actually added #trim to all text-input element types. I think that simply makes sense as a default for all text input elements in forms and results in consistency. :)

- Grepped for 'trim(' throughout core and replaced all instances that are obsolete.

So let's see what the bot thinks :)

Much better :)

AFAICS, there are two types of failures remaining:

1) ShortcutSetsTest + PageTitleFilteringTest are most likely bogus test expectations that simply need to be updated. (I didn't verify that though.)

2) The FormTest on the password_confirm field... I don't know. Perhaps we should simply remove the #trim from #type 'password' for now, and look into that later? That would mean we'd have to revert all related trim() removals from the user register/login form validation handlers. Personally, I'd be more than happy with ignoring that for now (i.e., reverting #trim on #type password). The massive simplifications elsewhere across the code base are worth to have regardless of those password-specifics. :)

Reverted #trim for #type password.

Hopefully, that leaves us with #58.1) only.

 function shortcut_set_switch_validate($form, &$form_state) {
   if ($form_state['values']['set'] == 'new') {
-    // Check to prevent creating a shortcut set with an empty title.
-    if (trim($form_state['values']['label']) == '') {
-      form_set_error('new', t('The new set label is required.'));
-    }

And I can only guess that the Shortcut test is looking for this custom error message :)

However, with the new #trim behavior, the default #required message of "!name field is required." will be shown. :)

Thus, the test needs to be adjusted.

--

PageTitleFilteringTest, OTOH, is a security test, so might be doing something very spooky ;) Needs analysis.

@aaron: Can you look into those two tests? :)

why not try the following?

function shortcut_set_switch_validate($form, &$form_state) {
   if ($form_state['values']['set'] == 'new') {
    // Check to prevent creating a shortcut set with an empty title.
    if ($form_state['values']['label'] == '') {
      form_set_error('new', t('The new set label is required.'));
    }

Well, the purpose of this patch is to remove all the custom trim() handling that was previously required. In turn, that means that the behavior of the system is slightly changed. The (current) behavior is covered by tests though, in order to verify our current expectations.

But due to this patch, our expectations are changing. We do not expect a form validation handler to have to worry about trimming submitted form values anymore, because the operation is performed centrally already. Due to the changed expectation, existing form validation handler code that previously threw a custom error message is no longer needed and thus won't throw that custom error message anymore. As a result, the expectation in our current tests has to be changed. :)

As predicted in #58, the only two remaining failures are in ShortcutSetsTest + PageTitleFilteringTest. The first one should really just be a trivial matter of adjusting the form validation error message string that the test apparently seems to look for. The page title test OTOH might try to verify some edge-case expectation; e.g., submitting a node with a title consisting of spaces only (or similar), which we need to look into; it's also perfectly possible that it's equally a trivial expectation change though. :)

It appears to be setting the title to

<span>&lt;/title&gt;&lt;script type=&quot;text/javascript&quot;&gt;alert(&quot;Title XSS!&quot;);&lt;/script&gt; &amp; &lt; &gt; &quot; \&#039;</span>

I do not know what it should be doing in regards to our trimming, so I post this for posterity.

I looked into both tests and corresponding code and adjusted them accordingly.

With that, I consider this patch ready.

It looks good to me now. Although, I wonder why you removed the assertion messages from that test. I would mark this RTBC, but I am unable to, being one of the authors of this patch.

I wanted to test and green-light this, but sorry, a few failures to apply today. looks like HEAD is moving too fast and this touched a lot of different files.
OTOH, maybe it's just me. re-queuing against todays code..

#65: drupal8.form-trim.65.patch queued for re-testing.

This patch is a re-roll against HEAD. There were some changes to the node/content-types.inc. I couldn't find any trim statements within menu/menu-admin.inc.

That test fail looks like it's probably genuine. Code in point looks like

    // Configure the Styles plugin, and ensure the updated settings are saved.
    $edit = array(
      'editor[settings][plugins][stylescombo][styles]' => "h1.title|Title\np.callout|Callout\n\n",
    );
    $this->drupalPost(NULL, $edit, t('Save configuration'));
    $expected_settings['plugins']['stylescombo']['styles'] = "h1.title|Title\np.callout|Callout\n\n";
    $editor = entity_load('editor', 'filtered_html');
    $this->assertTrue($editor instanceof Editor, 'An Editor config entity exists.');
    $this->assertIdentical($expected_settings, $editor->settings, 'The Editor config entity has the correct settings.');

- for some reason they WANTED those trailing newlines to be hanging around. Is that by design to raise a red flag incase something like this DOES unexpectedly trim the user input?

That's just whitespace garbage. Doesn't serve any purpose. Can we safely removed.

Done so in attached patch.

#73: drupal8.form-trim.73.patch queued for re-testing.

#73: drupal8.form-trim.73.patch queued for re-testing.

I'm not sure what's up with the bot. I am able to apply the patch just fine, however when I install Drupal with the patch applied, it does not log me in automatically. I am not certain, but I think that the bot is being confused by this, and giving us the wrong error message for some reason.

Removing unrelated tags

Let's try again (removing those config tags, which apparently are not removable)

Updated issue summary.

8 years since last update? safe to assume this issue is dead?

Just because focus may have shifted to other priorities and/or people not having time/energy/motivation, it doesn't mean that this issue "is dead". This is a very valid request, and a nice DX improvement + existing code cleanup. So still worth pursuing.

Since D8 is now EoL, I'm assuming that this needs to go against latest 9.x, so I've updated the issue metadata accordingly.

So..... how do I trim the email at the login page, is there a way? I am googling 30 min already.

UPD: Ok, I have figured this out