Deprecate civicrm_contribution_recur.trxn_id #21539
Conversation
|
(Standard links)
|
|
GoCardless, mentioned above, started off using I think this PR ought to also update the comment on the
Suggest:
The current text is wrong because: (1) it doesn't have to be unique. e.g. two different processors might use "sub0001" as an ID; it's probably unique within the domain of the third party processor, but it doesn't have to be unique in Civi's table. But I'm generally onboard, though I think we'll need an upgrade migration for - e.g. PayPal - if it's working differently now. |
| @@ -74,6 +74,9 @@ public function view() { | |||
| NULL, TRUE, NULL, FALSE, CRM_Core_Permission::VIEW); | |||
| CRM_Core_BAO_CustomGroup::buildCustomDataView($this, $groupTree, FALSE, NULL, NULL, NULL, $contributionRecur['id']); | |||
|
|
|||
| if ($contributionRecur['processor_id'] === $contributionRecur['trxn_id']) { | |||
There was a problem hiding this comment.
Do you need guards here against index not found?
| // we need to add a unique trxn_id to avoid a unique key error | ||
| // in paypal IPN we reset this when paypal sends us the real trxn id, CRM-2991 | ||
| $recurParams['trxn_id'] = $params['trxn_id'] ?? $params['invoiceID']; | ||
| $recurParams['processor_id'] = $recurParams['trxn_id'] = $params['trxn_id']; |
There was a problem hiding this comment.
We lost the functionality that if there's no trxn_id invoice_id is copied to trxn_id. If this doesn't matter, I think it needs documenting / commenting on as to why.
There was a problem hiding this comment.
Yes. I thought it didn't matter but looks like paypal may rely on it. Have updated the comment and left it in.
mattwire
force-pushed
the
deprecaterecurtrxnid
branch
2 times, most recently
from
095c0f4
to
f7114a3
Compare
mattwire
force-pushed
the
deprecaterecurtrxnid
branch
2 times, most recently
from
0f98621
to
d660361
Compare
mattwire
force-pushed
the
deprecaterecurtrxnid
branch
from
d660361
to
a1820dc
Compare
|
Merging per @artfulrobot review and so we don't have to regenerate the upgrader again. |
Overview
See discussion on https://lab.civicrm.org/dev/financial/-/issues/57
The
civicrm_contribution_recurtable has bothtrxn_idandprocessor_id. Various parts of the core code require that you useprocessor_id(eg. UpdateBilling/UpdateSubscription historically it was the only value passed that could be used to identify the recurring contribution).I'm not aware of any payment processor that actually uses both - it's basically impossible to do so because of the way core has handled them - and those that do rely on one of those fields generally copy the same value into both to make sure - eg. https://lab.civicrm.org/extensions/gocardless/-/blob/main/CRM/GoCardlessUtils.php#L389
Before
Not clear which parameter should be used.
After
Clearer..
Technical Details
Comments