Request new password problem

The problem is occurring inside the user_pass() function in user.module. It's easier to describe if you break the function up.

Part 1

First, the POST variables are tested. I think the code is supposed to function such that the conditional tests should only load a user into the $account variable if a corresponding username or email address is found in the database.

function user_pass() {
  global $base_url;
  $edit = $_POST['edit'];

  if ($edit['name'] && !($account = user_load(array('name' => $edit['name'], 'status' => 1)))) {
    form_set_error('name', t('Sorry. The username %name is not recognized.', array('%name' => theme('placeholder', $edit['name']))));
  }
  else if ($edit['mail'] && !($account = user_load(array('mail' => $edit['mail'], 'status' => 1)))) {
    form_set_error('mail', t('Sorry. The e-mail address %email is not recognized.', array('%email' => theme('placeholder', $edit['mail']))));
  }

Part 2

That way the following conditional should only result in a mail being sent if the correct information is available.

  if ($account) {

    $from = variable_get('site_mail', ini_get('sendmail_from'));
    $pass = user_password();

    // Save new password:
    user_save($account, array('pass' => $pass));

    // Mail new password:
    $variables = array('%username' => $account->name, '%site' => variable_get('site_name', 'drupal'), '%password' => $pass, '%uri' => $base_url, '%uri_brief' => substr($base_url, strlen('http://')), '%mailto' => $account->mail, '%date' => format_date(time()), '%login_uri' => url('user', NULL, NULL, TRUE), '%edit_uri' => url('user/'. $account->uid .'/edit', NULL, NULL, TRUE));
    $subject = _user_mail_text('pass_subject', $variables);
    $body = _user_mail_text('pass_body', $variables);
    $headers = "From: $from\nReply-to: $from\nX-Mailer: Drupal\nReturn-path: $from\nErrors-to: $from";
    $mail_success = user_mail($account->mail, $subject, $body, $headers);

    if ($mail_success) {
      watchdog('user', t('Password mailed to %name at %email.', array('%name' => theme('placeholder', $account->name), '%email' => theme('placeholder', $account->mail))));
      drupal_set_message(t('Your password and further instructions have been sent to your e-mail address.'));
    }

Part 3

This is the code that runs if the test fails and there is insufficient correct data to change the password and inform the user.

    else {
      watchdog('user', t('Error mailing password to %name at %email.', array('%name' => theme('placeholder', $account->name), '%email' => theme('placeholder', $account->mail))), WATCHDOG_ERROR);
      drupal_set_message(t('Unable to send mail. Please contact the site admin.'));
    }
    drupal_goto('user');
  }
  else {
    if ($edit) {
      drupal_set_message(t('You must provider either a username or e-mail address.'), 'error');
    }
    // Display form:
    $output = '<p>'. t('Enter your username <strong><em>or</em></strong> your e-mail address.') .'</p>';
    $output .= form_textfield(t('Username'), 'name', $edit['name'], 30, 64);
    $output .= form_textfield(t('E-mail address'), 'mail', $edit['mail'], 30, 64);
    $output .= form_submit(t('E-mail new password'));
    print theme('page', form($output));
  }
}

The problem is that user_load() will always return an object, so $account will always be instantiated in Part 1, rendering the second part of each conditional test redundant. An empty user gets assigned if a real user cannot be found in the database. The net effect is that execution will always pass into Part 2 and not into Part 3 if the data supplied in the form is wrong rather than just empty.

The MySQL error is generated because user_save() is called in Part 2 on an empty user, which tries to insert a new record into users with the default username (an empty string), clashing with the username of the null user (uid=0) and violating the constraint requiring usernames to be unique.

I have fixed the problem by changing Part 1 to look like this:

function user_pass() {
  global $base_url;
  $edit = $_POST['edit'];

  if ($edit['name']) {
    $account = user_load(array('name' => $edit['name'], 'status' => 1));
    
    if (!$account->uid) {
      form_set_error('name', t('Sorry. The username %name is not recognized.', array('%name' => theme('placeholder', $edit['name']))));
    }
  }
  else if ($edit['mail']) {
    $account = user_load(array('mail' => $edit['mail'], 'status' => 1));
    
    if (!$account->uid) {
      form_set_error('mail', t('Sorry. The e-mail address %email is not recognized.', array('%email' => theme('placeholder', $edit['mail']))));
    }
  }

And the start of Part 2 to this:

  if ($account->uid) {

Based on this bug report I suspect that the problem may be limited to php 5 (I'm using php 5.0.4 with drupal 4.6.3). I have never looked at the drupal codebase before today so I haven't tested it with php 4. All I know is that the demo site doesn't manifest this problem. I also don't know whether my workaround is a good fix or not as I don't know the code well enough to know whether there are any hidden implications. But it seems to work for me.

Can someone let me know whether I should submit this as a proper bug with the fix?