From 6a78439ab622e8a284ed76ff1abc699069724368 Mon Sep 17 00:00:00 2001
From: Rich Lott / Artful Robot <forums@artfulrobot.uk>
Date: Sun, 14 Jun 2020 23:05:54 +0100
Subject: [PATCH] Payment PropertyBag now issues warning if array access used
 to get custom prop

---
 Civi/Payment/PropertyBag.php                  | 23 +++++++++++++++----
 .../phpunit/Civi/Payment/PropertyBagTest.php  | 20 ++++++++++++----
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php
index 5bb9c55381..97d8f03430 100644
--- a/Civi/Payment/PropertyBag.php
+++ b/Civi/Payment/PropertyBag.php
@@ -121,10 +121,19 @@ class PropertyBag implements \ArrayAccess {
    * @return mixed
    */
   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;
+    try {
+      $prop = $this->handleLegacyPropNames($offset);
+    }
+    catch (InvalidArgumentException $e) {
+      $this->legacyWarning($e->getMessage() . " ArrayAccess used to access non-standard property. Please rewrite your code to use PropertyBag->getCustomProperty if it is a genuinely custom property, or a standardised getter like PropertyBag->getContactID() for standard properties");
+      try {
+        return $this->getCustomProperty($offset, 'default');
+      }
+      catch (BadMethodCallException $e) {
+        $this->legacyWarning($e->getMessage() . " calling getCustomProperty on a non-set property will result in a BadMethodCallException exception, but for your legacy use we have returned NULL. Please update your code.");
+        return NULL;
+      }
+    }
     return $this->get($prop, 'default');
   }
 
@@ -244,7 +253,7 @@ class PropertyBag implements \ArrayAccess {
    * @return mixed
    */
   protected function get($prop, $label) {
-    if (array_key_exists($prop, $this->props['default'])) {
+    if (array_key_exists($prop, $this->props[$label] ?? [])) {
       return $this->props[$label][$prop];
     }
     throw new \BadMethodCallException("Property '$prop' has not been set.");
@@ -1060,6 +1069,10 @@ class PropertyBag implements \ArrayAccess {
     if (isset(static::$propMap[$prop])) {
       throw new \InvalidArgumentException("Attempted to get '$prop' via getCustomProperty - must use using its getter.");
     }
+
+    if (!array_key_exists($prop, $this->props[$label] ?? [])) {
+      throw new \BadMethodCallException("Property '$prop' has not been set.");
+    }
     return $this->props[$label][$prop] ?? NULL;
   }
 
diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php
index d9eb8c0cea..dc0a7fc74f 100644
--- a/tests/phpunit/Civi/Payment/PropertyBagTest.php
+++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php
@@ -122,10 +122,11 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
     // Test we can do this with array, although we should get a warning.
     $propertyBag = new PropertyBag();
     $propertyBag['customThingForMyProcessor'] = 'fidget';
+    $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);
     $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);
+    $this->assertEquals("Unknown property 'customThingForMyProcessor'. ArrayAccess used to access non-standard property. Please rewrite your code to use PropertyBag->getCustomProperty if it is a genuinely custom property, or a standardised getter like PropertyBag->getContactID() for standard properties", $propertyBag->lastWarning);
   }
 
   /**
@@ -140,7 +141,9 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
   }
 
   /**
-   * Test we can't get a custom prop that was not set.
+   * Test we get NULL for custom prop that was not set.
+   *
+   * This is only for backward compatibility/ease of transition. One day it would be nice to throw an exception instead.
    *
    * @expectedException \BadMethodCallException
    * @expectedExceptionMessage Property 'aCustomProp' has not been set.
@@ -352,9 +355,16 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt
       $this->assertEquals("Attempted to get 'contribution_recur_id' via getCustomProperty - must use using its getter.", $e->getMessage());
     }
 
-    // Nb. hmmm. the custom property getter does not throw an exception if the property is unset, it just returns NULL.
-    $result = $propertyBag->getter('something_custom');
-    $this->assertNull($result, "Failed testing the getter on an unset custom property when not providing a default");
+    // Nb. up to 5.26, the custom property getter did not throw an exception if the property is unset, it just returned NULL.
+    // Now, we return NULL for array access (legacy) but for modern access
+    // (getter, getPropX(), getCustomProperty())  then we throw an exception if
+    // it is not set.
+    try {
+      $result = $propertyBag->getter('something_custom');
+      $this->fail("Expected a BadMethodCallException when getting 'something_custom' which has not been set.");
+    }
+    catch (\BadMethodCallException $e) {
+    }
 
     try {
       $propertyBag->setter('some_custom_thing', 'foo');
-- 
2.25.1