From aa29942d08a4fbe2e4b0a66d514e0d77124fcb31 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Thu, 18 Jun 2020 15:30:55 +0100 Subject: [PATCH] PropertyBag now uses deprecatedFunctionWarning instead of log; is silent during cast() calls --- Civi/Payment/PropertyBag.php | 81 ++++++----- .../phpunit/Civi/Payment/PropertyBagTest.php | 126 ++++++++++++++---- 2 files changed, 152 insertions(+), 55 deletions(-) diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php index bf94a02b0d..49a4ea4e8c 100644 --- a/Civi/Payment/PropertyBag.php +++ b/Civi/Payment/PropertyBag.php @@ -3,6 +3,7 @@ namespace Civi\Payment; use InvalidArgumentException; use Civi; +use CRM_Core_Error; use CRM_Core_PseudoConstant; /** @@ -21,11 +22,6 @@ use CRM_Core_PseudoConstant; * */ class PropertyBag implements \ArrayAccess { - /** - * @var array - * - see legacyWarning - */ - public static $legacyWarnings = []; protected $props = ['default' => []]; @@ -75,6 +71,14 @@ class PropertyBag implements \ArrayAccess { 'isNotifyProcessorOnCancelRecur' => TRUE, ]; + + /** + * @var bool + * Temporary, internal variable to help ease transition to PropertyBag. + * Used by cast() to suppress legacy warnings. + */ + protected $suppressLegacyWarnings = FALSE; + /** * Get the property bag. * @@ -125,19 +129,30 @@ class PropertyBag implements \ArrayAccess { $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"); + + CRM_Core_Error::deprecatedFunctionWarning( + "proper getCustomProperty('$offset') for non-core properties. " + . $e->getMessage(), + "PropertyBag array access to get '$offset'" + ); + try { return $this->getCustomProperty($offset, 'default'); } catch (BadMethodCallException $e) { - \CRM_Core_Error::deprecatedFunctionWarning( - "Use \$propertyBag->setCustomProperty('$offset', \$value) and then \$propertyBag->getCustomProperty('$offset') instead.", - "Accessing a not-set custom property via array access is deprecated. We're returning NULL for now but you should update your code." + CRM_Core_Error::deprecatedFunctionWarning( + "proper setCustomProperty('$offset', \$value) to store the value (since it is not a core value), then access it with getCustomProperty('$offset'). NULL is returned but in future an exception will be thrown." + . $e->getMessage(), + "PropertyBag array access to get unset property '$offset'" ); - $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; } } + + CRM_Core_Error::deprecatedFunctionWarning( + "get" . ucfirst($offset) . "()", + "PropertyBag array access for core property '$offset'" + ); return $this->get($prop, 'default'); } @@ -157,7 +172,15 @@ class PropertyBag implements \ArrayAccess { // This is fine if it's something particular to a payment processor // (which should be using setCustomProperty) however it could also lead to // things like 'my_weirly_named_contact_id'. - $this->legacyWarning($e->getMessage() . " 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"); + // + // From 5.28 we suppress this when using PropertyBag::cast() to ease transition. + if (!$this->suppressLegacyWarnings) { + CRM_Core_Error::deprecatedFunctionWarning( + "proper setCustomProperty('$offset', \$value) for non-core properties. " + . $e->getMessage(), + "PropertyBag array access to set '$offset'" + ); + } $this->setCustomProperty($offset, $value, 'default'); return; } @@ -173,6 +196,12 @@ class PropertyBag implements \ArrayAccess { // These lines are here (and not in try block) because the catch must only // catch the case when the prop is custom. $setter = 'set' . ucfirst($prop); + if (!$this->suppressLegacyWarnings) { + CRM_Core_Error::deprecatedFunctionWarning( + "$setter()", + "PropertyBag array access to set core property '$offset'" + ); + } $this->$setter($value, 'default'); } @@ -186,24 +215,6 @@ class PropertyBag implements \ArrayAccess { unset($this->props['default'][$prop]); } - /** - * Log legacy warnings info. - * - * @param string $message - */ - protected function legacyWarning($message) { - if (empty(static::$legacyWarnings)) { - // First time we have been called. - register_shutdown_function([PropertyBag::class, 'writeLegacyWarnings']); - } - // Store warnings instead of logging immediately, as calls to Civi::log() - // can take over half a second to work in some hosting environments. - static::$legacyWarnings[$message] = TRUE; - - // For unit tests: - $this->lastWarning = $message; - } - /** * Save any legacy warnings to log. * @@ -244,7 +255,10 @@ class PropertyBag implements \ArrayAccess { throw new \InvalidArgumentException("Unknown property '$prop'."); } // Remaining case is legacy name that's been translated. - $this->legacyWarning("We have translated '$prop' to '$newName' for you, but please update your code to use the propper setters and getters."); + if (!$this->suppressLegacyWarnings) { + CRM_Core_Error::deprecatedFunctionWarning("Canonical property name '$newName'", "Legacy property name '$prop'"); + } + return $newName; } @@ -309,16 +323,21 @@ class PropertyBag implements \ArrayAccess { /** * This is used to merge values from an array. - * It's a transitional function and should not be used! + * It's a transitional, internal function and should not be used! * * @param array $data */ public function mergeLegacyInputParams($data) { + // Suppress legacy warnings for merging an array of data as this + // suits our migration plan at this moment. Future behaviour may differ. + // @see https://github.com/civicrm/civicrm-core/pull/17643 + $this->suppressLegacyWarnings = TRUE; foreach ($data as $key => $value) { if ($value !== NULL && $value !== '') { $this->offsetSet($key, $value); } } + $this->suppressLegacyWarnings = FALSE; } /** diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php index dc0a7fc74f..0a66c6dd7c 100644 --- a/tests/phpunit/Civi/Payment/PropertyBagTest.php +++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php @@ -3,6 +3,7 @@ namespace Civi\Payment; use Civi\Test\HeadlessInterface; use Civi\Test\TransactionalInterface; +use PHPUnit\Framework\Error\Deprecated as DeprecatedError; /** * @group headless @@ -58,31 +59,47 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt */ public function testSetContactIDLegacyWay() { $propertyBag = new PropertyBag(); - $propertyBag['contactID'] = 123; - $this->assertEquals(123, $propertyBag->getContactID()); - $this->assertEquals(123, $propertyBag['contactID']); - // There should not be any warnings yet. - $this->assertEquals("", $propertyBag->lastWarning); - // Now access via legacy name - should work but generate warning. - $this->assertEquals(123, $propertyBag['contact_id']); - $this->assertEquals("We have translated 'contact_id' to 'contactID' for you, but please update your code to use the propper setters and getters.", $propertyBag->lastWarning); + // To prevent E_USER_DEPRECATED errors during phpunit tests we take a copy + // of the existing error_reporting. + $oldLevel = error_reporting(); + $ignoreUserDeprecatedErrors = $oldLevel & ~E_USER_DEPRECATED; - // Repeat but this time set the property using a legacy name, fetch by new name. - $propertyBag = new PropertyBag(); - $propertyBag['contact_id'] = 123; - $this->assertEquals("We have translated 'contact_id' to 'contactID' for you, but please update your code to use the propper setters and getters.", $propertyBag->lastWarning); - $this->assertEquals(123, $propertyBag->getContactID()); - $this->assertEquals(123, $propertyBag['contactID']); - $this->assertEquals(123, $propertyBag['contact_id']); + foreach (['contactID', 'contact_id'] as $prop) { + // Set by array access should cause deprecated error. + try { + $propertyBag[$prop] = 123; + $this->fail("Using array access to set a property '$prop' should trigger deprecated notice."); + } + catch (DeprecatedError $e) { + } + + // But it should still work. + error_reporting($ignoreUserDeprecatedErrors); + $propertyBag[$prop] = 123; + error_reporting($oldLevel); + $this->assertEquals(123, $propertyBag->getContactID()); + + // Getting by array access should also cause deprecation error. + try { + $_ = $propertyBag[$prop]; + $this->fail("Using array access to get a property '$prop' should trigger deprecated notice."); + } + catch (DeprecatedError $e) { + } + + // But again, it should work. + error_reporting($ignoreUserDeprecatedErrors); + $this->assertEquals(123, $propertyBag[$prop], "Getting '$prop' by array access should work"); + error_reporting($oldLevel); + } } /** * Test that emails set by the legacy method of 'email-5' can be retrieved with getEmail. */ public function testSetBillingEmailLegacy() { - $localPropertyBag = new PropertyBag(); - $localPropertyBag->mergeLegacyInputParams(['email-' . \CRM_Core_BAO_LocationType::getBilling() => 'a@b.com']); + $localPropertyBag = PropertyBag::cast(['email-' . \CRM_Core_BAO_LocationType::getBilling() => 'a@b.com']); $this->assertEquals('a@b.com', $localPropertyBag->getEmail()); } @@ -101,8 +118,7 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt /** */ public function testMergeInputs() { - $propertyBag = new PropertyBag(); - $propertyBag->mergeLegacyInputParams([ + $propertyBag = PropertyBag::cast([ 'contactID' => 123, 'contributionRecurID' => 456, ]); @@ -114,6 +130,10 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt * Test we can set and access custom props. */ public function testSetCustomProp() { + $oldLevel = error_reporting(); + $ignoreUserDeprecatedErrors = $oldLevel & ~E_USER_DEPRECATED; + + // The proper way. $propertyBag = new PropertyBag(); $propertyBag->setCustomProperty('customThingForMyProcessor', 'fidget'); $this->assertEquals('fidget', $propertyBag->getCustomProperty('customThingForMyProcessor')); @@ -121,12 +141,34 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt // Test we can do this with array, although we should get a warning. $propertyBag = new PropertyBag(); + + // Set by array access should cause deprecated error. + try { + $propertyBag['customThingForMyProcessor'] = 'fidget'; + $this->fail("Using array access to set an implicitly custom property should trigger deprecated notice."); + } + catch (DeprecatedError $e) { + } + + // But it should still work. + error_reporting($ignoreUserDeprecatedErrors); $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); + error_reporting($oldLevel); $this->assertEquals('fidget', $propertyBag->getCustomProperty('customThingForMyProcessor')); - // Test array access, too. + + // Getting by array access should also cause deprecation error. + try { + $_ = $propertyBag['customThingForMyProcessor']; + $this->fail("Using array access to get an implicitly custom property should trigger deprecated notice."); + } + catch (DeprecatedError $e) { + } + + // But again, it should work. + error_reporting($ignoreUserDeprecatedErrors); $this->assertEquals('fidget', $propertyBag['customThingForMyProcessor']); - $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); + error_reporting($oldLevel); + } /** @@ -150,7 +192,25 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt */ public function testGetCustomPropFails() { $propertyBag = new PropertyBag(); - $v = $propertyBag['aCustomProp']; + // Tricky test. We need to ignore deprecation errors, we're testing deprecated behaviour, + // but we need to listen out for a different exception. + $oldLevel = error_reporting(); + $ignoreUserDeprecatedErrors = $oldLevel & ~E_USER_DEPRECATED; + error_reporting($ignoreUserDeprecatedErrors); + + // Do the do. + try { + $v = $propertyBag['aCustomProp']; + error_reporting($oldLevel); + $this->fail("Expected BadMethodCallException from accessing an unset custom prop."); + } + catch (\BadMethodCallException $e) { + // reset error level. + error_reporting($oldLevel); + // rethrow for phpunit to catch. + throw $e; + } + } /** @@ -192,9 +252,14 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt $this->fail("Expected an error trying to set $prop to " . json_encode($given) . " but did not get one."); } + $oldLevel = error_reporting(); + $ignoreUserDeprecatedErrors = $oldLevel & ~E_USER_DEPRECATED; + // Check array access for the proper property name and any aliases. + // This is going to throw a bunch of deprecated errors, but we know this + // (and have tested it elsewhere) so we turn those off. + error_reporting($ignoreUserDeprecatedErrors); foreach (array_merge([$prop], $legacy_names) as $name) { - // Check array access foreach ($valid_values as $_) { list($given, $expect) = $_; $propertyBag = new PropertyBag(); @@ -205,6 +270,7 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt break; } } + error_reporting($oldLevel); } /** @@ -230,10 +296,15 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt public function testUtilsArray() { $propertyBag = new PropertyBag(); $propertyBag->setContactID(123); + // This will throw deprecation notices but we don't care. + $oldLevel = error_reporting(); + $ignoreUserDeprecatedErrors = $oldLevel & ~E_USER_DEPRECATED; + error_reporting($ignoreUserDeprecatedErrors); $this->assertEquals(123, \CRM_Utils_Array::value('contact_id', $propertyBag)); // Test that using utils array value to get a nonexistent property returns the default. $this->assertEquals(456, \CRM_Utils_Array::value('ISawAManWhoWasntThere', $propertyBag, 456)); + error_reporting($oldLevel); } /** @@ -250,6 +321,12 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt $propertyBag->setCustomProperty('custom_zls', ''); $propertyBag->setCustomProperty('custom_0', 0); + // To prevent E_USER_DEPRECATED errors during phpunit tests we take a copy + // of the existing error_reporting. + $oldLevel = error_reporting(); + $ignoreUserDeprecatedErrors = $oldLevel & ~E_USER_DEPRECATED; + error_reporting($ignoreUserDeprecatedErrors); + // Tests on known properties. $v = empty($propertyBag->getContactID()); $this->assertFalse($v, "empty on a set, known property should return False"); @@ -279,6 +356,7 @@ class PropertyBagTest extends \PHPUnit\Framework\TestCase implements HeadlessInt $v = empty($propertyBag['custom_issue']); $this->assertFalse($v, "empty on a set custom property accessed by ArrayAccess should return False"); + error_reporting($oldLevel); } /** -- 2.25.1