CRM-19963 - Use translated processor name for Paypal in all comparisons

[rthouvenin]

I guess a better would idea would be not to use a translatable string as identifier, but this is a bigger change and I was not sure of the consequences. This small patch makes sure PayPal works in all languages (problem noticed in German).

---

• CRM-19963: Paypal Express not working in German

[civicrm-builder]

Can one of the admins verify this patch?

[eileenmcnaughton]

This seems OK to me - but you will need to create a new PR against master. The process is that we fork for the rc & from that point on we only permit new PRs into the rc if they address a new regression in the rc. If in doubt always do your PR against master.

[eileenmcnaughton]

NB - it seems that if we are having to consider translation we are probably comparing against the wrong field?

[rthouvenin]

Thanks @eileenmcnaughton .
Instead of creating a new PR, I rebased my branch and changed the target of the PR so that we keep this discussion. But now I realise the label 4.7.16-rc should be removed. Can this be done, or should I just scrap this and open a new PR?

Regarding comparing against the wrong field: I agree, this was the essence of my description of the PR, but like I said I am not familiar enough with the codebase to evaluate what are all the places where this comparison is done and needs to be changed.

[eileenmcnaughton]

Just thinking about this - this is one of those types of fixes that could have unintended consequences and the one I'm wondering about is a translated site which has not translated the processor names. Does it make sense to be if( $name = ts($paymentProcessorName) || $paymentProcessorName)

[eileenmcnaughton]

Just looking I think the 'right' fix would be to add a label field to the payment_processor table & make 'name' not translated

[rthouvenin]

I think the case of a translated site with no translation for processor names would already have similar issues. It doesn't show on the diff of this PR, but there are other such comparisons of processor names, and they do use the translated string. For example: https://github.com/WeMoveEU/civicrm-core/blob/aef7f33ad79a6868f684fa89617d10183c090575/CRM/Core/Payment/PayPalImpl.php#L1065

See also the assignments in the constructor of the class.
Actually, maybe an easier 'right' fix would be to use the same comparison as the constructor, everywhere.

[totten]

I had the same initial reaction as eileen ("does it make sense to be if( $name = ts($paymentProcessorName) || $paymentProcessorName"). But a quick grep confirms @rthouvenin's points. To wit, here's a grep on status-quo code:

$ rgrep _processorName CRM
...
CRM/Core/Payment/PayPalImpl.php:    if ($this->_processorName == ts('PayPal Express') || $this->_processorName == ts('PayPal Pro')) {
CRM/Core/Payment/PayPalImpl.php:    if ($this->_processorName == 'PayPal Express' || $this->_processorName == 'PayPal Pro') {

You can't serve both -- one is right, one is wrong. The ts() version looks right (based on skimming the other grep outputs), and this PR makes the two checks the same.