From: eileen <emcnaughton@wikimedia.org>
Date: Mon, 11 May 2020 23:25:53 +0000 (+1200)
Subject: Permit empty processorID
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=5b59c71980c35aa299d77a57be4a2ade82083887;p=civicrm-core.git

Permit empty processorID

On deploying this code I'm seeing InvalidArgumentException: 'processorID field has max length of 255'
---

diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php
index c067a1f77a..d17c04b845 100644
--- a/Civi/Payment/PropertyBag.php
+++ b/Civi/Payment/PropertyBag.php
@@ -227,9 +227,11 @@ class PropertyBag implements \ArrayAccess {
    *
    * @param mixed $prop Valid property name
    * @param string $label e.g. 'default'
+   *
+   * @return mixed
    */
   protected function get($prop, $label) {
-    if (isset($this->props['default'][$prop])) {
+    if (array_key_exists($prop, $this->props['default'])) {
       return $this->props[$label][$prop];
     }
     throw new \BadMethodCallException("Property '$prop' has not been set.");
@@ -599,11 +601,13 @@ class PropertyBag implements \ArrayAccess {
   /**
    * @param int $contributionRecurID
    * @param string $label e.g. 'default'
+   *
+   * @return \Civi\Payment\PropertyBag
    */
   public function setContributionRecurID($contributionRecurID, $label = 'default') {
     // We don't use this because it counts zero as positive: CRM_Utils_Type::validate($contactID, 'Positive');
     if (!($contributionRecurID > 0)) {
-      throw new InvalidArgumentException("ContributionRecurID must be a positive integer");
+      throw new InvalidArgumentException('ContributionRecurID must be a positive integer');
     }
 
     return $this->set('contributionRecurID', $label, (int) $contributionRecurID);
@@ -916,7 +920,9 @@ class PropertyBag implements \ArrayAccess {
    * Nb. this is stored in civicrm_contribution_recur.processor_id and is NOT
    * in any way related to the payment processor ID.
    *
-   * @return string
+   * @param string $label
+   *
+   * @return string|null
    */
   public function getRecurProcessorID($label = 'default') {
     return $this->get('recurProcessorID', $label);
@@ -926,12 +932,20 @@ class PropertyBag implements \ArrayAccess {
    * Set the unique payment processor service provided ID for a particular
    * subscription.
    *
-   * @param string $input
+   * See https://github.com/civicrm/civicrm-core/pull/17292 for discussion
+   * of how this function accepting NULL fits with standard / planned behaviour.
+   *
+   * @param string|null $input
    * @param string $label e.g. 'default'
+   *
+   * @return \Civi\Payment\PropertyBag
    */
   public function setRecurProcessorID($input, $label = 'default') {
-    if (empty($input) || strlen($input) > 255) {
-      throw new \InvalidArgumentException("processorID field has max length of 255");
+    if ($input === '') {
+      $input = NULL;
+    }
+    if (strlen($input) > 255 || in_array($input, [FALSE, 0], TRUE)) {
+      throw new \InvalidArgumentException('processorID field has max length of 255');
     }
     return $this->set('recurProcessorID', $label, $input);
   }
diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php
index e058325366..cc31e0427c 100644
--- a/tests/phpunit/Civi/Payment/PropertyBagTest.php
+++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php
@@ -9,19 +9,13 @@ use Civi\Test\TransactionalInterface;
  */
 class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInterface, TransactionalInterface {
 
+  /**
+   * @return \Civi\Test\CiviEnvBuilder
+   */
   public function setUpHeadless() {
     return \Civi\Test::headless()->apply();
   }
 
-  protected function setUp() {
-    parent::setUp();
-    // $this->useTransaction(TRUE);
-  }
-
-  public function tearDown() {
-    parent::tearDown();
-  }
-
   /**
    * Test we can set a contact ID.
    */
@@ -80,6 +74,18 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
     $this->assertEquals(123, $propertyBag['contact_id']);
   }
 
+  /**
+   * Test that null is valid for recurring contribution ID.
+   *
+   * See https://github.com/civicrm/civicrm-core/pull/17292
+   */
+  public function testRecurProcessorIDNull() {
+    $bag = new PropertyBag();
+    $bag->setRecurProcessorID(NULL);
+    $value = $bag->getRecurProcessorID();
+    $this->assertNull($value);
+  }
+
   /**
    */
   public function testMergeInputs() {
@@ -235,7 +241,7 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
       ['paymentToken', [], $valid_strings, []],
       ['recurFrequencyInterval', ['frequency_interval'], $valid_ints, $invalid_ints],
       ['recurFrequencyUnit', [], [['month', 'month'], ['day', 'day'], ['year', 'year']], ['', NULL, 0]],
-      ['recurProcessorID', [], [['foo', 'foo']], [str_repeat('x', 256), NULL, '', 0]],
+      ['recurProcessorID', [], [['foo', 'foo']], [str_repeat('x', 256)]],
       ['transactionID', ['transaction_id'], $valid_strings, []],
       ['trxnResultCode', [], $valid_strings, []],
     ];