Duplicate first name appearing in SagePay transaction listings

One of our customers was experiencing a similar issue, i.e. instances where a persons first name was appearing twice on SagePay, on the Live SagePay server.

I have been able to test the patch using a development copy of the site, with the Sagepay test server. So far I have seen no instances of the repeated first names on the test transactions that have been completed.

I will need to test with some live transactions next, before confirming whether or not the patch fixes the issue.

I can confirm I'm hitting this issue too , it looks like the version in the repo fixes this in a different way by checking the number of elements in the split_name array.
Im wondering if the naming/logic of this could be looked at. its basically saying if Ive got a name line, overwrite firstname and lastname. Could there be edge cases where you'll be overwriting correct data ?

function _commerce_sagepay_format_customer_name($name_line, $first_name, $last_name) {

  // If we have been supplied with a $name_line variable, split it by space
  // and make the assumption that the last word is the last name and all other
  // are the first name.
  $name = array();

  if (isset($name_line)) {
    $split_name = explode(' ', trim($name_line));

    $last_name = array_pop($split_name);
    if (count($split_name) > 1) {
      foreach ($split_name as $n) {
        $first_name .= $n;
        $first_name .= ' ';
      }
    } else {
      $first_name = array_pop($split_name);
    }

    // Allow for a situation where only last name was entered on the name line.
    if ($first_name == '') {
      $first_name = 'not set';
    }
  }

  $name['first_name'] = trim($first_name);
  $name['last_name'] = trim($last_name);

  return $name;
}

this appears to be the commit that fixes it

commit b86cc5183a5d5ddff3d4c0a141e5d8c00c80fa22
Author: abasso <abasso@841514.no-reply.drupal.org>
Date:   Fri May 23 06:10:01 2014 +0100

    Issue #2250003 by Koozer: First name duplicated on submit to Sage Pay

I think the problem is that the function doesn't actually overwrite the $first_name variable - it concatenates to it.

I could be wrong but it seems like Addressfield is now copying first_name and last_name into name_line. If you have your address widgets set to use the separate first_name and last_name fields, when this function is called you'll get duplicate data in the variables.

If it's determined to use the same variable, it needs to scrub it first ($first_name = '';). Though as it happens there is a PHP function designed to do what that foreach() loop is doing, so we can just replace the whole thing with implode(). And, actually, that removes the need for a count() check. TBH, I'd drop the overwrite in favour of writing directly to the return array:

function _commerce_sagepay_format_customer_name($name_line, $first_name, $last_name) {
  // If we have been supplied with a $name_line variable, split it by space
  // and make the assumption that the last word is the last name and all other
  // are the first name.
  $name = array();

  if (isset($name_line)) {
    $split_name = explode(' ', trim($name_line));

    $name['last_name'] = array_pop($split_name);
    $name['first_name'] = implode(' ', $split_name);

    // Allow for a situation where only last name was entered
    if ($first_name == '') {
      $name['first_name'] = 'not set';
    }
  }
  else {
    $name['first_name'] = trim($first_name);
    $name['last_name'] = trim($last_name);
  }

  return $name;
}

(No idea whether addressfield does the reverse when the single name_line is in use, btw. For what it's worth, I think it's doing it to preserve data when switching between the widget settings, so it's not impossible.)

All of that said, however - this "always using the name_line" principle may be causing issues with name lengths. See #2312791: The BillingFirstnames value is too long.

Actually, on reflection, name_line is now always going to be present so there's no real point in that whole switch.

Maybe this would be better:

function _commerce_sagepay_format_customer_name($name_line = '', $first_name = '', $last_name = '') {

  $name = array(
    'first_name' => '',
    'last_name' => '',
  );

  if (!empty($first_name) || !empty($last_name)) {
    // If we have been supplied with a first and last name, use these in
    // preference to the name_line.
    $name['first_name'] = trim($first_name);
    $name['last_name'] = trim($last_name);
  }
  elseif (!empty($name_line)) {
    // If we have been supplied with a $name_line variable, split it by space
    // and make the assumption that the last word is the last name and all other
    // are the first name.
    $split_name = explode(' ', trim($name_line));

    $name['last_name'] = array_pop($split_name);
    $name['first_name'] = implode(' ', $split_name);

    // Allow for a situation where only last name was entered.
    if ($name['first_name'] == '') {
      $name['first_name'] = 'not set';
    }
  }

  return $name;
}

Sorry to swamp everyone's inboxes with updates, but I've confirmed that since the change to Addressfield, it's always populating name_line, first_name and last_name on save (though unlike commerce_sagepay it interprets a single entered value as first_name). I honestly think the sagepay code can safely use the first_name and last_name properties from the address value, no formatting required. We could actually drop the _commerce_sagepay_format_customer_name() function altogether, but for completeness:

function _commerce_sagepay_format_customer_name($name_line = '', $first_name = '', $last_name = '') {

  $name = array(
    'first_name' => empty($first_name) ? 'not set' : $first_name,
    'last_name' => empty($last_name) ? 'not set' : $last_name,
  );

  return $name;
}

For reference, here's the Addressfield change, from 2012. #1659066: Update all name columns when a name is entered in either full name or first / last name form elements

OK this looks good. My only concern is backwards compatibility is people haven't updated that address field module. Since it was a year ago, I expect we'll be ok.

I'll do a change note when this goes on to note that the updated version of Address field is a dependency.

Let me do some testing that removes this function all together.

Thanks for digging around on this one!

R

Closing now as duplicate of https://www.drupal.org/node/2312791