CiviCRM Community Forums (archive)

we had the same problem and hacked our way through with a hook.

In general, the payment processor framework makes a lot of assumptions that might not apply all the time indeed. Would you have the time to improve it?

ie, having methods in the payment processor that are called to set/alter things like the status or contribution type instead of being hard coded?

That doesn't feel right to me - I think core should be patched so that the payment processor can return the status of the initial contribution

Okay, so here's the bigger picture: the fact that there's no way to set the status to 1 for recurring payments without this back door is really a symptom of how the payment processor subsystem was created. i.e. a few payment processors were created for the most popular ones, and there was an attempt to generalize some code into the base payment processor object and implement the details of each payment processor by subclassing it. Presumably in the case of recurring payments, all the initial examples used some kind of call back (IPN?) to verify the recurring payments, so that was not considered to be something that needed to be overrideable in the doDirectPayment method, etc.

When I look at the diversity of ways that payment gateways work these days, I think that this initial attempt (it was 7 years ago and hasn't really been updated since) needs a rethink. In particular, it seems a bit too monolithic, and too separate from the recording of payments. I'd like to see a more flexible framework that enforces the recording of the payments to be connected with the payment processor. Something that would allow an extension to more simply build new payment pages that can more easily reflect what the payment gateway can do, rather than trying to overgeneralize a basic payment page logic that tries to be everything. Right now, building a new payment page is almost as scary as tinkering with the default one.

Okay, so here's the bigger picture: the fact that there's no way to set the status to 1 for recurring payments without this back door is really a symptom of how the payment processor subsystem was created. i.e. a few payment processors were created for the most popular ones, and there was an attempt to generalize some code into the base payment processor object and implement the details of each payment processor by subclassing it.

Agree and agreed it isn't finished and doesn't work well for recurring payments. (for the record, I think "generic" is mostly well done by refactorisation once you got enough use cases, not by overthinking beforehand based on assumptions over one use case: it tends to create a rube goldberg or abstract pattern decorator factory pattern observer pattern)

Anyway, what's the way forward? wouldn't a setStatus () {return 1;} in the parent class PP that can be overrided by each PP if they want something different be a solution?

X+

Getting back to my original quest from which I'd hijacked my own thread ...

The way to fix the contribution_status_id relatively easily in the existing world is to just use the hook_civicrm_pre, which is designed to let you tamper with records just before they get written.

So you just have to implement hook_civicrm_pre for op = 'create', and objectName = 'Contribution' or 'ContributionRecur'.

The only slightly tricky bit is that the params variable for the initial contribution doesn't actually contain the payment processor id, so you have to dig back by looking it up using the payment page id, which you do have.  And if you do try this at home also note that a call to this api:

$result = civicrm_api('ContributionPage', 'getsingle', $params);
will return the payment processor id with key 'payment_processor', and not 'payment_processor_id' as you would expect.

In case that code gets messy and you just want to understand how to do this for your own code, this commit
https://github.com/adixon/ca.civicrm.iats/commit/bcc66d0af74d9c23b35db99ddb88a7856b1e2d27
should be the useful bit you need (it has a bit of extra noise in there, but look at everything after the iats_civicrm_pre function.)

And now, re-hijacking my thread: I think a real simple solution to avoid even this amount of extra code would be to just allow doDirectPayment to set the status value. I remembered that was the first thing I tried, and if the code in the processCreditCard just checked before setting the status, I think that would be the easiest fix. But I hesitate to provide a patch because figuring out the code flow is a bit of nightmare and I suspect it's more complicated than I know ...