From 22dc3ee32f5ac266c784e1dc03e142b1ef1d8bed Mon Sep 17 00:00:00 2001
From: Rich Lott / Artful Robot <forums@artfulrobot.uk>
Date: Fri, 12 Jun 2020 10:41:33 +0100
Subject: [PATCH] Improve PropertyBag handling of offsetGet and custom
 properties; add more tests

---
 Civi/Payment/PropertyBag.php                  | 11 +++-
 .../phpunit/Civi/Payment/PropertyBagTest.php  | 63 ++++++++++++++++++-
 2 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php
index a97c8d113f..5bb9c55381 100644
--- a/Civi/Payment/PropertyBag.php
+++ b/Civi/Payment/PropertyBag.php
@@ -109,7 +109,9 @@ class PropertyBag implements \ArrayAccess {
    */
   public function offsetExists ($offset): bool {
     $prop = $this->handleLegacyPropNames($offset, TRUE);
-    return $prop && isset($this->props['default'][$prop]);
+    // If there's no prop, assume it's a custom property.
+    $prop = $prop ?? $offset;
+    return array_key_exists($prop, $this->props['default']);
   }
 
   /**
@@ -118,8 +120,11 @@ class PropertyBag implements \ArrayAccess {
    * @param mixed $offset
    * @return mixed
    */
-  public function offsetGet ($offset) {
-    $prop = $this->handleLegacyPropNames($offset);
+  public function offsetGet($offset) {
+    $prop = $this->handleLegacyPropNames($offset, TRUE);
+    // Allow transparent access to custom properties.
+    // We could issue a deprecation notice here, but we've probably got enough in this code already!
+    $prop = $prop ?? $offset;
     return $this->get($prop, 'default');
   }
 
diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php
index f5c7b59e28..d9eb8c0cea 100644
--- a/tests/phpunit/Civi/Payment/PropertyBagTest.php
+++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php
@@ -13,7 +13,10 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
    * @return \Civi\Test\CiviEnvBuilder
    */
   public function setUpHeadless() {
-    return \Civi\Test::headless()->apply();
+    static $reset = FALSE;
+    $return = \Civi\Test::headless()->apply($reset);
+    $reset = FALSE;
+    return $return;
   }
 
   /**
@@ -120,6 +123,8 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
     $propertyBag = new PropertyBag();
     $propertyBag['customThingForMyProcessor'] = 'fidget';
     $this->assertEquals('fidget', $propertyBag->getCustomProperty('customThingForMyProcessor'));
+    // Test array access, too.
+    $this->assertEquals('fidget', $propertyBag['customThingForMyProcessor']);
     $this->assertEquals("Unknown property 'customThingForMyProcessor'. We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a standardised setter like PropertyBag->setContactID for standard properties", $propertyBag->lastWarning);
   }
 
@@ -134,6 +139,17 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
     $propertyBag->setCustomProperty('contactID', 123);
   }
 
+  /**
+   * Test we can't get a custom prop that was not set.
+   *
+   * @expectedException \BadMethodCallException
+   * @expectedExceptionMessage Property 'aCustomProp' has not been set.
+   */
+  public function testGetCustomPropFails() {
+    $propertyBag = new PropertyBag();
+    $v = $propertyBag['aCustomProp'];
+  }
+
   /**
    *
    * @dataProvider otherParamsDataProvider
@@ -217,6 +233,51 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
     $this->assertEquals(456, \CRM_Utils_Array::value('ISawAManWhoWasntThere', $propertyBag, 456));
   }
 
+  /**
+   */
+  public function testEmpty() {
+    $propertyBag = new PropertyBag();
+    $propertyBag->setContactID(123);
+    $propertyBag->setRecurProcessorID('');
+    $propertyBag->setBillingPostalCode(NULL);
+    $propertyBag->setFeeAmount(0);
+    $propertyBag->setCustomProperty('custom_issue', 'black lives matter');
+    $propertyBag->setCustomProperty('custom_null', NULL);
+    $propertyBag->setCustomProperty('custom_false', FALSE);
+    $propertyBag->setCustomProperty('custom_zls', '');
+    $propertyBag->setCustomProperty('custom_0', 0);
+
+    // Tests on known properties.
+    $v = empty($propertyBag->getContactID());
+    $this->assertFalse($v, "empty on a set, known property should return False");
+    $v = empty($propertyBag['contactID']);
+    $this->assertFalse($v, "empty on a set, known property accessed by ArrayAccess with correct name should return False");
+    $v = empty($propertyBag['contact_id']);
+    $this->assertFalse($v, "empty on a set, known property accessed by ArrayAccess with legacy name should return False");
+    $v = empty($propertyBag['recurProcessorID']);
+    $this->assertTrue($v, "empty on an unset, known property accessed by ArrayAccess should return True");
+    $v = empty($propertyBag->getRecurProcessorID());
+    $this->assertTrue($v, "empty on a set, but '' value should return True");
+    $v = empty($propertyBag->getFeeAmount());
+    $this->assertTrue($v, "empty on a set, but 0 value should return True");
+    $v = empty($propertyBag->getBillingPostalCode());
+    $this->assertTrue($v, "empty on a set, but NULL value should return True");
+
+    // Test custom properties.
+    $v = empty($propertyBag->getCustomProperty('custom_issue'));
+    $this->assertFalse($v, "empty on a set custom property with non-empty value should return False");
+    foreach (['null', 'false', 'zls', '0'] as $_) {
+      $v = empty($propertyBag["custom_$_"]);
+      $this->assertTrue($v, "empty on a set custom property with $_ value should return TRUE");
+    }
+    $v = empty($propertyBag['nonexistent_custom_field']);
+    $this->assertTrue($v, "empty on a non-existent custom property should return True");
+
+    $v = empty($propertyBag['custom_issue']);
+    $this->assertFalse($v, "empty on a set custom property accessed by ArrayAccess should return False");
+
+  }
+
   /**
    *
    * Data provider for testOtherParams
-- 
2.25.1