Call validatePaymentInstrument instead of validateCreditCard directly

[mattwire]

This replaces PR #88 as I did that on the main branch...

validateCreditCard should not be called directly, but should be called via the overrideable base class in CRM_Core_Payment_Form::validatePaymentInstrument.

This was breaking stripe via webform as we don't validate credit card fields for stripe (a token is passed instead).

[KarinG]

This looks good and I've confirmed all is well with iATS Payments Credit Card and iATS Payments ACHEFT/Direct Debit - on 4.6.33; Eileen confirmed this looks good for 4.7;

[KarinG]

This turned out to be a breaking change for 4.7.27 -> can't pass the credit card validation -> for Expiry Date; reverting this solves the problem the webform can process payments again. Problem was experienced using iATS Payments but almost certainly affects other non-stripe inline payment processors as well.

[KarinG]

@mattwire - any thought on how we can fix this such that it will work both for Stripe as well as for inline payment processors [in 4.7];

[mattwire]

@KarinG hey, I'm away this week but will look at it early next week and get my head around why it's failing.

[KarinG]

Awesome - thank you; enjoy your break!

[mattwire]

@KarinG I haven't used IATS credit card before. However, I've just set it up with webform and test account details from http://home.iatspayments.com/developer-info/testing/#2999. It seems to work fine for me with civicrm master and webform_civicrm master. Can you give me any more detail as to when it fails?
By the way, the change in this PR just switches to using the "standard" CiviCRM validation instead of hardcoded validation in webform_civicrm - this means that extensions can override it if they need to (and that's what Stripe is doing as we don't validate the CC details on submit (as Stripe.js has already done that and generated a token if they are valid).

[KarinG]

Like I said - it's balking at the Expiry date - validation:

With this PR:

After rolling it back:

All is fine; I've not had a chance to track down/grep where that Validation kicks in - but have seen this with two 4.7 clients now.

[mattwire]

Previously validateCreditCard was being called directly.
Now, we are calling validatePaymentInstrument (https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L418) which means we additionally call validateMandatoryFields() - it is that call which is now failing because (I think) iats is stripping the expiry date (via javascript?) before submitting the form.

So I think the solution is to implement validatePaymentInstrument in CRM_Core_Payment_iATSService? (I see there is an implementation in CRM_Core_Payment_iATSServiceSWIPE).

See examples here: https://github.com/mattwire/org.civicrm.smartdebit/blob/master/CRM/Core/Payment/Smartdebit.php#L180 and https://github.com/mattwire/com.drastikbydesign.stripe/blob/4.7-mjwconsulting-dev-squashed/CRM/Core/Payment/Stripe.php#L902

[KarinG]

We're definitively sending over Expiry Date to iATS in order to transact - one simply can not transact a credit card online without it.

[mattwire]

@KarinG Sorry, wasn't sure how IATS was working, but I see it is passing expiry date to civicrm.

However, I'm now unable to reproduce the issue - using webform_civicrm 7.x-4.x master with CiviCRM 4.7.27 and master it always passes validation (though I then get a failure from IATS with 9002: General decline code. Please have cardholder call the number on the back of the card.)

[mattwire]

@KarinG I do see the following PHP notices though:

Notice: Undefined index: month in CRM_Core_Payment_iATSService->convertParams() (line 373 of /home/dev/civicrm-buildkit/build/dmaster/sites/default/files/civicrm/ext/com.iatspayments.civicrm/CRM/Core/Payment/iATSService.php).
Notice: Undefined index: year in CRM_Core_Payment_iATSService->convertParams() (line 373 of /home/dev/civicrm-buildkit/build/dmaster/sites/default/files/civicrm/ext/com.iatspayments.civicrm/CRM/Core/Payment/iATSService.php).

Seems that convertParams is looking for $params['month'] and $params['year'] but they can be found in $params['credit_card_exp_date']['M'] and $params['credit_card_exp_date']['Y']

Adding the following in convertParams got rid of the notices and passed the correct expiry date (instead of 00/00) to IATS:

    $params['month'] = $params['credit_card_exp_date']['M'];
    $params['year'] = $params['credit_card_exp_date']['Y'];

[mattwire]

@eileenmcnaughton FYI ^^ in case you have any thoughts?

[KarinG]

So to summarize: all our 4.7.27 sites transact happily through civicrm-native pathways; but their webform-civicrm forms can no longer process credit cards with this PR. Could be a case where params have changed and are different coming through different pathways - but if we’re using old style params - certainly other inline payment processor may be too as they work just fine in civicrm-native; so I’m not convinced editing our iATS master is the solution.

[rekaeps]

We ran into this issue just yesterday using the latest versions of everything and Authorize.net.

[KarinG]

Thank you for adding that - my recommendation is that we roll this back

[mattwire]

@KarinG @rekaeps Please can you try the patch in PR #106?

Expiry date was not being passed to the validation function by webform_civicrm. Note that when PR #100 is merged issues like this should go away as all of the hardcoded fields in that function goes away and civicrm native stuff is used instead.

[KarinG]

Let’s revert this PR and then add it back into #106; keeping functionally related bits together

[rekaeps]

Expiry error went away but I got a 'website encountered an unexpected error' after webform submission (using test card numbers). CiviCRM still received registration and webform logged it. Payment was also logged in CiviCRM. Checking Drupal's logs, I saw this line: CiviCRM_API3_Exception: entity_id is not a valid integer in civicrm_api3() (line 45 of /srv/www/cliniclegal.org/public_html/sites/all/modules/civicrm/api/api.php).

At this point, assuming patch works and this is specific to my installation. Not a developer, so not able to look into more details.

[KarinG]

@rekaeps - we've reverted this patch; you can now safely use master branch for your 4.7 - we've asked @mattwire to add this PR to his #106 - I'm not convinced that your Fatal error is specific to your instance; #106 becomes a bucket for everything else that needs to be changed when we switch to validatePaymentInstrument