CRM-20949 - Fix payment processor assignment

[jitendrapurohit]

• CRM-20949: BillingBlock.tpl contains wrong paymentProcessor object on initial load of backend Contribution page.

[jitendrapurohit]

$processor is the variable that iterates in the foreach loop written above and assigns the last iterated value to the paymentProcessor var. $this->_paymentProcessor holds the correct value that needs to be assigned.

[eileenmcnaughton]

@jitendrapurohit does the $mode parameter affect this - in theory it should be pay_later processor if mode is not set

[jitendrapurohit]

The handling for $mode not set is already done above the for loop. So I guess what we get in $this->_paymentProcessor at the end of this function is a correctly calculated value.

[eileenmcnaughton]

@jitendrapurohit I just looked at this again - does this have any symptoms other than an incorrect assignment? I ask because the todo says to remove those lines.

I will merge this as I agree it is more correct, & if the code is going to stay we should make it right. But I'd prefer to understand why it is an issue.

[eileenmcnaughton]

(I would also note there are other tangental bugs to getting this more correct may help with them)

[jitendrapurohit]

Thanks @eileenmcnaughton - I recall we had the overridden copy of BillingBlock.tpl where the usage of $paymentProcessor worked correctly. It failed due to this inaccurate assignment.

With the removal of this line - I don't see $paymentProcessor var to be available on offline New Contribution or New Credit Card Contribution forms which may affect any external tpl files using this variable.

[eileenmcnaughton]

Ok - that's a marginal reason for keeping the line - overridden tpls is not something core needs to be worry about & we actively discourage people from doing that.

Having said that I think we can let this drop for now. I'm happy that the merge was at least more correct in than out.

[jitendrapurohit]

+1 for the first statement. I just fixed as the assignment looked wrong to me in the code. But the above explanation might contradict the requirement of this PR - #10757. Feel free to close if it doesn't make any sense to you. I might find a different way to obtain that result ;-).

[eileenmcnaughton]

That PR is already merged & yeah - it's a bit grey - because while I don't particularly support tpl overrides you could equally be using that data in a hook (supported).

The bottom line is if you may an obscure core change to support an extension usage & want it to not break then unit tests are your friend

[petednz]

@jitendrapurohit pinged me to say this had been applied on a site where having a non-credit card set as default results in Direct Debit fields showing on a Pledge, even though a CC processor shows in the selector.
As far as I can see this has made no difference to the site in question. I still get Direct Debit fields.
Am going to resolve issue for that client but setting no default processor.