The password and password_confirm fields do not handle empty values very well (at least in D6). When you submit a settings form that contains a password / password_confirm field, you have to enter the password every time you make a change or it will reset the variable entry to empty string. It seems like this applies to any situation where you have module settings that automatically get saved to variables when the form is submitted.

This is resulting in bugs (issue 3 below) and also questionable practices (the passwords and some API keys in the other ubercart payment gateway modules are textfield instead of password because of this).

Although the User edit account form behaves well, it is not the same kind of form as these settings forms, so the same solution doesn't really apply very well. Really, it shouldn't require any (or at least any significant) work to make your password field retain its old selection if you don't enter a new value. I'm fine with adding a new field parameter that indicates the field is "optional" in this sense, where a new empty value results in no change.

Some relevant issues: 1 2 3

CommentFileSizeAuthor
#1 password_test.tar_.gz737 byteskwinters
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kwinters’s picture

kwinters CreditAttribution: kwinters commented
Title: password / password_confirm fields do not handle empty values very well » system_settings_form does not support password / password_confirm fields well
Version: 6.x-dev » 7.x-dev
Status: Active » Needs work
FileSize
password_test.tar_.gz737 bytes

Installed D7 and wrote a tiny module to confirm that the problem exists there as well (attached).

If someone can give me some guidance about what an appropriate fix for this would be, I can do the legwork. The two approaches I can think of right now are modifying system_settings_form not change the value if the POST for that field was empty and it was type password / password_confirm OR modifying the form system to return the default value in some case (probably requires a new option).

Also related: password_confirm does not support #default_value at all (Fatal error: Unsupported operand types in includes/form.inc on line 1312)

kwinters’s picture

kwinters CreditAttribution: kwinters commented

Forgot to leave instructions on using the module.

After enabling it, go to admin/settings/password_test and fill out the form, then submit. The 3 variables will be saved correctly to the database. You've been redirected back to the form, which should now be filled in with the actual data. Submit the form again, and it will blow out your password settings.

phayes’s picture

phayes CreditAttribution: phayes commented

I think the correct behavior would be to edit the system_settings_form processor and not update any password fields that are left blank.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Had this issue myself on a new module I'm building...
here's a way to do this without changing core Drupal...

function example_admin_settings() {
  $form = array();

  /* Some settings form with password field */

  $form['example_password'] = array(
    '#type' => 'password',
    '#title' =>  t('Password'),
    '#default_value' => variable_get('example_password', ''),
    '#description' => t('Enter the password to connect'),
  );


  $form = system_settings_form($form);
  //Add custom submit handler so that passwords will not be overwritten if saved with no value
  array_unshift($form['#submit'], 'example_admin_settings_submit');

  return $form;
}

function example_admin_settings_submit($form, &$form_state) {
  if ($form_state['values']['example_password'] == '') {
    unset($form_state['values']['example_password']);
  }
}
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

@phayes: then how would you erase an existing non-required value? I've a gut feeling that the password field needs additional logic, looking at the bare field it just seems its creators expected implementors would intuit the need for further custom logic.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs work » Closed (won't fix)

system_settings_form() is being removed in D8 (see #1696224: Remove system_settings_form()) so I'm marking this wont fix.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

@heyrocker: Is it not worth pursuing for D7?

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Closed (won't fix) » Active

Ack I just noticed this was a D7 issue, not a D8 one. My bad.

kwinters’s picture

kwinters CreditAttribution: kwinters commented

Also, does whatever replaces it in D8 actually fix the issue? The problem may be at the form layer instead of unique to system_settings_form().

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

There is no replacement in D8, you just build your forms as normal and write your own submit functions.

jakubmrozonline’s picture

jakubmrozonline CreditAttribution: jakubmrozonline commented
Issue summary: View changes

@alexpott: Works well in D7.x. Thanks :) After 7 years, LOL :)

djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin at Cadmium commented

We had way too many forms and used contribs with their own forms, so I wrote a module to address this because doing it manually was really not an option.

https://www.drupal.org/project/config_form_passwords

Password fields on forms that use system_settings_form() or manually specified forms will not be reset if they are left empty.