No support for Maestro, Laser, Diners, Solo

I've used this patch to add Maestro support to a site and it works brilliantly. The code looks good too -- I'm not sure how the patch applies (I manually made the changes, since it's so small) but it seems good functionally.

One minor nitpick:

-      ($id == 6 && !variable_get('uc_credit_discover', TRUE)) ||
-      !ctype_digit($number)) {
+      ($id == 6 && !variable_get('uc_credit_discover', TRUE) && !variable_get('uc_credit_maestro', TRUE)) ||
+    !ctype_digit($number)) {

I'd split that big conjunction / disjunction over lines like the rest of the statement.

Cheers for the wonderful work :), can we get some feedback from someone with commit access?

Come on guys -- let's get this in to the next RC!

Currently there's no support for Maestro, Laser, Diners, Switch or Solo cards. There's probably other ones missing too, but figured I'd start with this set first as these are the ones supported by my payment gateway.

The attached patch adds supports for these cards and provides the necessary icons for the checkout page.

Apparently Switch was renamed to Maestro, so updating patch.

"Diners" should be "Diners Club" in the admin UI. Otherwise, definite +1 from me.

Tested, works as expected.

Updated patch file which includes proper checking of the CVV length. The last one was way too open when Maestro, etc were enabled, as they do not require a CVV and if enabled any length CVV was allowed for every card type. However this required removing the selection box configuration and instead building the CC types options from the checkboxes selected for the accepted cards. It makes more sense to me this way - you get stronger CVV validation and don't have to enter in select options for the credit cards, and you could extend it to build stronger number validation too.

This part of the credit card handling has always been a little weak, but the suggested fix seems like a hack rather than a solution. There's always going to be "just one more" card that needs to be added. Plus, simply checking the "Maestro" (or any other card type) box on the admin page doesn't mean your store can now accept Maestro cards. Sure, Ubercart will then put the card logo on the checkout page, but that doesn't mean any particular payment gateway can process that card type.

The real problem isn't that certain cards aren't listed, the real problem is that Ubercart doesn't have a mechanism that can be used to add types without hacking core. To do it right, payment gateways should be able to register new card types with Ubercart through a hook (for lack of a better term :-) That way, the only card types that will show up in the admin menu are ones actually supported by the gateway, and Ubercart will be able to handle any card type defined by a gateway. No further patches needed, no matter how many new card types are invented. Win win.

I would also like to either get rid of the first digit check entirely or fix it to check the complete issuer identification number (IIN), which is the first six digits. Maestro can start with 5 or 6, Switch can start with 4, 5, or 6, Diners can start with 2, 3 or 5. One digit alone isn't enough to determine if the entered number is a type we can accept. The patch doesn't get this right since it assumes Diners is always 3 and Maestro is always 6, for instance. Likewise, since both Discover and Maestro can start with 6, it's impossible to support Maestro and reject Discover - we can't tell them apart by just the first digit.

There are really three things this section of code is meant to do:
1) Allow the admin to select which credit card types to accept.
2) Present a logo to the customer at checkout to inform him/her what cards may be used for payment.
3) Validate that the entered card number is one of the types defined in #1, before Ubercart sends the card information to the gateway for processing.

New card types added via a hook_form_alter() can accomplish #1 and #2. It's only #3 that poses a problem (and the original topic of this thread was to fix #3 for Maestro). If uc_credit had a hook instead, payment gateways could supply a logo, card name, and IIN range, and # of CVV digits for the card types they support.

Subscribe

I don't think it's necessarily fair to say this patch is a hack. It does extend the list of supported card types and strengthens the CCV checking. However, it is definitely a work in progress.

I agree the card number bin range checking could be substantially improved - perhaps we could learn from the Pay module here.

As for a hook which each payment gateway implements to identify the card types they support, I think this is a great idea! My only concern is that we're changing the api and each of those payment gateway modules (not all of them are within ubercart package) need to be updated too and the overhead/hassle of that. I feel that introducing such a hook would delay this functionality ever seeing the light of day in the 6.x-2.x branch. Having said that maybe we could introduce a default set (Visa, Mastercard) to fall back on...

My other concern with a hook, while it allows payment gateways to extend the list of supported cards, you're pushing the card validation onto each gateway. Specifying the length is not a big deal, easy enough to get right, but card bin ranges vary a lot and what if one payment gateway gets the bin range right for Visa and another one doesn't? I feel we should have a central piece of code for defining bin ranges, but then you're back to having to modify ubercart each time a new card type needs to be supported.

I'm happy to work on this, if people think it would get supported to the 6.x-2.x branch. I need support for these extra cards but don't want to have to run a patched version of ubercart for ever more.

The patch in #8 will break compatibility with the Sage Pay module (uc_protx_vsp_direct) as they require the customer to separately specify Visa, Visa Debit and Visa Electron, for which we must have separate entries in the accepted types text field.

I am not really sure why Ubercart should be trying to validate bin ranges at all; surely this will be taken care of by the payment gateway anyway - if the store doesn't accept Amex, then the gateway will return a suitable error (not that uc_credit shows these at the moment..). Isn't validating card numbers by the Luhn check alone enough, leaving the "selected card types" as a purely cosmetic option for checkout? Also, is there ever a real world use case for accepting Visa but not Mastercard or vice versa?

I'm between two minds on the validating card numbers item. I see it as being useful so you don't have to connect to the payment gateway to attempt a transaction that you could have determined would fail. However, it does add more complexity to the code.

I didn't realise that about Sage Pay, but then if we went with the new hook TR suggested, each payment gateway could provide their own list of supported card types. It would also specify the CCV length (I still want that), but could optionally define a bin range. If a bin range is provided we could validate against that, otherwise do no validation and leave the gateway handle it.

To not break existing payment gateways I guess ubercart should provide a default set of Visa, Mastercard (maybe AMEX and discover too?) to fall back on when a payment gateway doesn't support it? Not sure if we need to do this or not.

So to take TR's hook idea further, how about a hook implementation something like the following (hook name might need some work):

function hook_uc_credit_payment_accepted_card_types() {
  return array(
    'visa' => array(
      'label' => t('Visa'),
      'icon' => drupal_get_path('module', 'mygateway') . '/icons/visa.png',
      'has_ccv' => TRUE,
      'ccv_length' => 3,
      'cc_length' => array(16),
      'iin_range => array(4),
    ),
    'mastercard' => array(
      'label' => t('Mastercard'),
      'icon' => drupal_get_path('module', 'mygateway') . '/icons/mastercard.png',
      'has_ccv' => TRUE,
      'ccv_length' => 3,
      'cc_length' => array(16),
      'iin_range => array('51-55'),
    ),
    'amex' => array(
      'label' => t('American Express'),
      'icon' => drupal_get_path('module', 'mygateway') . '/icons/amex.png',
      'has_ccv' => TRUE,
      'ccv_length' => 4,
      'cc_length' => array(15),
      'iin_range => array(34, 37),
    ),
    'laser' => array(
      'label' => t('Laser'),
      'icon' => drupal_get_path('module', 'mygateway') . '/icons/laser.png',
      'has_ccv' => FALSE,
      'cc_length' => array(16, 17, 18, 19),
      'iin_range => array(6304, 6706, 6771, 6709),
    ),
  );
}

subscribing

The patch in #8 is the right solution for Ubercart 2, I believe. It fixes the immediate limitation by simply expanding the list of cards supported by core (which could be construed to be an arbitrary limitation).

Until someone comes up with a better solution to load additional card types, I think we should just patch core to recognise any and all card types that people need to accept.

The patch in #8 doesn't work properly for me though, as it requires that the card type selection option be enabled, which I don't use on any of my stores. It's basically just cosmetic.

Ubercart could detect the card type in the scenario that the user didn't provide it. The card number prefix will give you the card type.

I'm now running #8 but with _valid_cvv() reverted to the core version, and it's working nicely for me.

Does the fix in #8 validates "Visa" altogether as: Visa CC, Visa Debit and Visa Electron?

Marked #642474: Adding new "Accepted card types" - Adding Laser as duplicate.

For those that need to reapply #8 to 6.x-2.9, here it is, forward-ported.

Rerolled patch to apply on top of 2.13.

This won't be changed in 6.x-2.x. Moving to the most recent version of Ubercart as the payment architecture is currently being worked on there.