webform_civicrm
webform_civicrm copied to clipboard
colemanw
Remove Contribution.transact API from webform_civicrm 7.x
Overview
This pull request is intended to remove the contribute.transact API from webform_civicrm 7.x so that the contributiontransactapi extension is no longer required with it. Note I have tested this on webforms 7.x with stripe test payment processor. Could not test on iATS payment processor as of now.
Before
Before these changes, it required the contributiontransactapi extension to complete the transaction.
After
Now, it can complete transactions without contributiontransactapi extension. The changes are very much similar to what was done for webforms 8.x-5.x .
Technical Details
If the PR introduces noteworthy technical changes, please describe them here. Provide code snippets if necessary
Comments
Anything else you would like the reviewer to note
One reason we have not ported this change back to D7 is because it affects thousands of sites and D7 is EOL in 6mo so our approach has been to be very careful and minimize potentially breaking chances.
Maybe with proper testing through live payments using different processors, we can look to merge it. Are there still chances of breaking it? Then Is it possible to release it in an entirely different version then. Maybe 7.x-6.x? @mattwire what do you think?
Another issue is that this PR is actually quite different from the 8.x -> #434 ->
@KarinG It should look more like #437 - you did the initial refactor in #434 and then I did #437 to match desired behaviour
Hold on... there was a follow up PR by Matt after I initially added #434 (which was a straight up transaction API replacement that I tested on iATS and added a test to secure it).
@kartik1000 - so is this a straight up copy/paste of what's currently in 8.x-5.x - If there are any differences - can you please highlight them and explain them?
Perhaps a 7.x-6.x is possible once D7 becomes LTS (in Nov 2021). I think at that point we can help D7 sites using D7WFC LTS by removing code that relies on deprecations.
@KarinG. There is no difference inside the definition of function wf_civicrm_api3_contribution_transact from #437, the only difference is I am not fetching utils.incagain before calling wf_civicrm_api3_contribution_transact. This was working ok for me so I did not feel the need of changing anything.
I have now tested this with Stripe and confirm that it is working well. That is expected as it is the same code that went into 8.x and that is in the contributiontransactlegacy extension which was required (until this PR is merged) when using stripe with webform 7.x.
I touched base with @colemanw and he agrees we should minimize risk re: 7.x-5.x
I'd like to propose we wait until D7 is EOL in just a few months before we merge this. Sites continuing on with a commercially available D7 LTS at that time are already assuming some risk by having extended their D7 lifetime - and ongoing refactoring to remove deprecation notices in my opinion would fit within that risk.
But projects in the middle of upgrading to D9 do not need a potentially breaking payment processor change in their D7WFC. There are many payment processors who have never needed (and their WFC worked just fine) without contributiontransactlegacy extension - who is to say this PR will not cause any breaking changes for sites using those?
A bit confused by comment
I'd like to propose we wait until D7 is EOL in just a few months before we merge this.
Drupal 7 will reach end-of-life in November of 2022
Ah yes - I have my D8 and D7 mixed up. D8 hits EOL in Nov2021; D7 EOL was extended from Nov2021 to Nov2022.