ensure recur options are present on backend cc contribution form.

[jmcclelland]

Overview

CiviCRM seems to be adding an extra check to see if the given payment processor has any credit card fields before including the recur option on the backend contribution page. But some processors, like Stripe, insert the fields via javascript. It should be enough to just test for the existence of a payment processor that supports recur to run the block.

Before

When using the stripe payment processor via the backend, there is no option to set a recurring contribution.

After

After applying the patch, it appears:

Technical Details

Messing with this kind of code is somewhat terrying. And... I really don't know why this check was put there in the first place.

Also tagging @mattwire - as far as I can tell, the ability to add recurring contributions via the back end has not been possible for a while, but I'm honestly not sure. This discovery came up somewhat randomly for us.

Also, despite the complicated looking diff, the only non-white space change introduced is to remove this if clause:

if (CRM_Core_Payment_Form::buildPaymentForm($this, $this->_paymentProcessor, FALSE, TRUE, $this->payment_instrument_id) == TRUE)

Instead, we simply rely on if (!empty($this->_recurPaymentProcessors)) { to determine whether or not to include the recur block.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@jmcclelland I think maybe CRM_Core_Payment_Form::buildPaymentForm should return TRUE if it gets to the end of the function & FALSE if it early exists here

I think the whole point of the IF is to faciliate that early exit. (and we should add a bool type hint - true vs null is silly)

[jmcclelland]

Oh I see. I agree - that NULL should be FALSE in your snippet (and I can update my pull request to include that and a type hint). But I don't think it will solve the problem.

It seems like buildPaymentForm should build the payment form (or not build it) - but shouldn't be expected to do double duty and also inform whether or not the recur options show up on the backend contribution page.

Maybe my PR should keep CRM_Core_Payment_Form::buildPaymentForm($this, $this->_paymentProcessor, FALSE, TRUE, $this->payment_instrument_id); - just not use it's return code to determine whether buildRecurBlock is set to TRUE or FALSE.

For the record - Stripe does not get caught in the early return that you reference. It overrides buildForm - but does not return anything, which is interpreted as false-like. So, it goes all the way to the bottom of the function and returns FALSE because $form->_paymentFields is empty.

[eileenmcnaughton]

@jmcclelland since this is the only place the return value is used I guess if it doesn't make sense for it to return anything if it's not helpful to this form.

I think I'm OK with this then - @mattwire ?

[mattwire]

Thanks @jmcclelland this was on my list "to look at one day". We need to keep the call to buildPaymentForm() because otherwise CRM_Core_Payment_Xx::buildForm() is not called for the default payment processor on first load. So can you update the PR to call that but remove the if statement/TRUE check?

It would be nice to iterate this further in future PRs to simplify the code further and work out why/where buildForm() is called from on subsequent billingblock loads but not on the initial load

[jmcclelland]

Thanks for the feedback. I put the call to buildPaymentForm back in to ensure Stripe gets built properly.

And I removed the return values from that function since they are not being used anywhere now.

[mattwire]

Thanks @jmcclelland