dev/financial/#16 Paypal unreliable getting payment processor type

[mattwire]

Overview

The standard _paymentProcessor array does not include the type name (payment_processor_type) but does include the payment processor type id (payment_processor_type_id). However, paypal pro is expecting it to be there (it is passed in via a standard online contribution page).

Before

PHP notice on some payment submissions (eg. via webform_civicrm) and use of "non-standard" param.

After

No PHP notice and uses standard parameter so it works with all payment submissions that use standard methods to populate the payment processor array ($this->_paymentProcessor).

Related: #12091

[eileenmcnaughton]

@mattwire makes sense - I guess the only worry would be if there is some case where it's not passed in - I looked at the Omnipay & see

  /**
   * Ensure payment processor type is set.
   *
   * It's kind of 'hacked on' to the payment_processor params normally but not when called form
   * the pay api.
   *
   * @throws \CiviCRM_API3_Exception
   */
  protected function ensurePaymentProcessorTypeIsSet() {
    if (!isset($this->_paymentProcessor['payment_processor_type'])) {
      $this->_paymentProcessor['payment_processor_type'] = civicrm_api3('PaymentProcessorType', 'getvalue', array(
        'id' => $this->_paymentProcessor['payment_processor_type_id'],
        'return' => 'name',
      ));
    }
  }

which doesn't totally re-assure me that 'payment_processor_type_id' is always set (as opposed to ONE of them being always set)

[eileenmcnaughton]

@mattwire I think you've added commits to this PR that belong in new PRs?

[mattwire]

@eileenmcnaughton Well spotted :-) I saw this (https://civicrm.stackexchange.com/questions/24316/paypal-pro-billing-address-state-province-country-not-being-passed) on stackexchange yesterday and got carried away with my fixing stuff... I think I'll be splitting this into 3 separate PRs.
Are you happy with the new CRM_Utils_System::deprecatedFunctionWarning?

[eileenmcnaughton]

@mattwire I think @totten tried to enhance deprecated output at some point - but I think he was trying to do it within this function - is this a better place?

  /**
   * Logs with an arbitrary level.
   *
   * @param mixed $level
   * @param string $message
   * @param array $context
   */
  public function log($level, $message, array $context = array()) {
    // FIXME: This flattens a $context a bit prematurely. When integrating
    // with external/CMS logs, we should pass through $context.
    if (!empty($context)) {
      if (isset($context['exception'])) {
        $context['exception'] = CRM_Core_Error::formatTextException($context['exception']);
      }
      $message .= "\n" . print_r($context, 1);

      if (CRM_Utils_System::isDevelopment() && CRM_Utils_Array::value('civi.tag', $context) === 'deprecated') {
        trigger_error($message, E_USER_DEPRECATED);
      }
    }
    CRM_Core_Error::debug_log_message($message, FALSE, '', $this->map[$level]);
  }

[totten]

I think @totten tried to enhance deprecated output at some point

Probably this: https://chat.civicrm.org/civicrm/pl/cf1p7hgkkprp9kthkosq98qrer

A few random thoughts:

• A standalone helper function is more pithy than the Civi::log...civi.tag=>deprecated.... Pithy is good.
• From r-code perspective, I don't like having that helper in CRM_Utils_System. That class is too heavy already, and deprecatedFunctionWarning() is qualitatively different from every other function in the class.
• My main hesitation with cf1p7hgkkprp9kthkosq98qrer was that findNonLogCaller()
  felt a little heavy-handed. It's nice how deprecatedFunctionWarning() doesn't have to dig as far back into the callstack.
• On the other hand, it's nice how cf1p7hgkkprp9kthkosq98qrer works with the existing coding convention.
• The scoping of the PR is confusing. The subject says that it's about payment_processor_type, but half the changes are actually modifying code-conventions in unrelated code.

[mattwire]

@totten

A standalone helper function is more pithy than the Civi::log...civi.tag=>deprecated.... Pithy is good.
From r-code perspective, I don't like having that helper in CRM_Utils_System. That class is too heavy already, and deprecatedFunctionWarning() is qualitatively different from every other function in the class.

Yes, I agree. I wasn't sure where to "dump" the function. Maybe CRM/Core/Error/Log.php would be a better place for it?

My main hesitation with cf1p7hgkkprp9kthkosq98qrer was that findNonLogCaller() felt a little heavy-handed. It's nice how deprecatedFunctionWarning() doesn't have to dig as far back into the callstack.
On the other hand, it's nice how cf1p7hgkkprp9kthkosq98qrer works with the existing coding convention.

We don't actually log deprecated very much so I don't think it's a big change to convention.

The scoping of the PR is confusing. The subject says that it's about payment_processor_type, but half the changes are actually modifying code-conventions in unrelated code.

Yes I know, I started digging and went off on multiple tangents. It needs splitting into a few PRs :-)

[mattwire]

@eileenmcnaughton For the standard "getters" / php_notices please see #12038

[mattwire]

@eileenmcnaughton This PR is now updated and should alleviate your previous concerns about payment_processor_type as it no longer matters if it is set or not. As a bonus we remove usage of deprecated function paymentProcessorType!

[mattwire]

Related issue: #12091

[eileenmcnaughton]

@mattwire some extra commits have snuck in

[mattwire]

@eileenmcnaughton Yes, I've been tracking #12091 as they are inter-related. If I drop the last commit perhaps we could get this PR merged and then refactor #12091 on top of the changes in this PR. As I see that issues with "payment_processor_type" not being set are still happening on that PR whereas they would not once this was merged. @cartbar

[eileenmcnaughton]

@mattwire thanks - will wait on @cartbar testing feedback.

I did read through the code & it made sense from a code POV. I'd kinda like to put in a deprecation notice but I think that has been left out of this PR as you have another PR open to discuss changing that.

[mattwire]

Closing as now included in #12091