From ea9fb730ad2571428b538b56465f95cf6a785796 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Tue, 8 Nov 2022 10:37:12 +0000 Subject: [PATCH 1/2] Handle dodgier calls to setBillingCountry in property bag re issue core/3918 --- Civi/Payment/PropertyBag.php | 67 +++++++++++++++++-- .../phpunit/Civi/Payment/PropertyBagTest.php | 65 +++++++++++++++++- 2 files changed, 126 insertions(+), 6 deletions(-) diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php index 2b39b8474302..c190258a145c 100644 --- a/Civi/Payment/PropertyBag.php +++ b/Civi/Payment/PropertyBag.php @@ -4,6 +4,7 @@ use InvalidArgumentException; use CRM_Core_Error; use CRM_Core_PseudoConstant; +use Civi\Api4\Country; /** * @class @@ -83,6 +84,12 @@ class PropertyBag implements \ArrayAccess { 'isNotifyProcessorOnCancelRecur' => TRUE, ]; + /** + * For unit tests only. + * + * @var array + */ + public $logs = []; /** * @var bool @@ -646,13 +653,55 @@ public function getBillingCountry($label = 'default') { * @param string $label e.g. 'default' */ public function setBillingCountry($input, $label = 'default') { - if (!is_string($input) || strlen($input) !== 2) { - throw new \InvalidArgumentException("setBillingCountry expects ISO 3166-1 alpha-2 country code."); + $warnings = []; + $munged = $input; + if (!is_string($input)) { + $warnings[] = 'Expected string'; + } + else { + if (!(strlen($input) === 2 && CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_Address', 'country_id', $input))) { + $warnings[] = 'Not ISO 3166-1 alpha-2 code.'; + } + } + + if ($warnings) { + // Try to munge. + if (empty($input)) { + $munged = ''; + } + else { + if ((is_int($input) || preg_match('/^\d+$/', $input))) { + // Got a number. Maybe it's an ID? + $munged = Country::get(FALSE)->addSelect('iso_code')->addWhere('id', '=', $input)->execute()->first()['iso_code'] ?? ''; + if ($munged) { + $warnings[] = "Given input matched a country ID, assuming it was that."; + } + else { + $warnings[] = "Given input looked like it could be a country ID but did not match a country."; + } + } + elseif (is_string($input)) { + $munged = Country::get(FALSE)->addSelect('iso_code')->addWhere('name', '=', $input)->execute()->first()['iso_code'] ?? ''; + if ($munged) { + $warnings[] = "Given input matched a country name, assuming it was that."; + } + else { + $warnings[] = "Given input did not match a country name."; + } + } + else { + $munged = ''; + $warnings[] = "Given input is plain weird."; + } + } } - if (!CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_Address', 'country_id', $input)) { - throw new \InvalidArgumentException("setBillingCountry expects ISO 3166-1 alpha-2 country code."); + + if ($warnings) { + $warnings[] = "Input: " . json_encode($input) . " was munged to: " . json_encode($munged); + $this->logWarning(__FUNCTION__ . " input warnings (may be errors in future):\n" . implode("\n", $warnings)); } - return $this->set('billingCountry', $label, (string) $input); + + return $this->set('billingCountry', $label, $munged); } /** @@ -1208,4 +1257,12 @@ public function setCustomProperty($prop, $value, $label = 'default') { $this->props[$label][$prop] = $value; } + /** + * For unit tests only. + */ + protected function logWarning($message) { + $this->logs[] = $message; + \Civi::log()->warning($message); + } + } diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php index 67696aa425e3..e26a466b99d7 100644 --- a/tests/phpunit/Civi/Payment/PropertyBagTest.php +++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php @@ -429,7 +429,6 @@ public function otherParamsDataProvider() { ['billingPostalCode', ['billing_postal_code', 'postal_code', 'billing_postal_code-5'], $valid_strings_inc_null, []], ['billingCounty', [], $valid_strings_inc_null, []], ['billingStateProvince', ['billing_state_province', 'state_province', 'billing_state_province-5'], $valid_strings_inc_null, []], - ['billingCountry', [], [['GB', 'GB'], ['NZ', 'NZ']], ['XX', '', NULL, 0]], ['contributionID', ['contribution_id'], $valid_ints, $invalid_ints], ['contributionRecurID', ['contribution_recur_id'], $valid_ints, $invalid_ints], ['description', [], [['foo' , 'foo'], ['', '']], []], @@ -448,6 +447,70 @@ public function otherParamsDataProvider() { ]; } + /** + * Billing country is a mess. + */ + public function testBillingCountry() { + + // Test designed use + foreach (['NZ', 'GB'] as $valid) { + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry($valid); + $this->assertEquals($valid, $propertyBag->getBillingCountry()); + $this->assertCount(0, $propertyBag->logs); + } + + // Many sorts of nothingness + foreach ([NULL, 0, FALSE] as $bad) { + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry($bad); + $this->assertCount(1, $propertyBag->logs); + $latestLog = end($propertyBag->logs); + $this->assertRegExp("/setBillingCountry input warnings.*Expected string.*munged to: \"\"/s", $latestLog); + $this->assertEquals('', $propertyBag->getBillingCountry()); + } + + // '' special case + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry(''); + $this->assertCount(1, $propertyBag->logs); + $latestLog = end($propertyBag->logs); + $this->assertRegExp('/setBillingCountry input warnings.+\nNot ISO 3166-1.+\n.*munged to: ""/', $latestLog); + $this->assertEquals('', $propertyBag->getBillingCountry()); + + // Invalid country name + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry('UnitedKing'); + $this->assertCount(1, $propertyBag->logs); + $latestLog = end($propertyBag->logs); + $this->assertRegExp('/setBillingCountry input warnings.+\nNot ISO 3166-1.+\nGiven input did not match a country name\.\n.*munged to: ""/', $latestLog); + $this->assertEquals('', $propertyBag->getBillingCountry()); + + // Valid country name + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry('United Kingdom'); + $this->assertCount(1, $propertyBag->logs); + $latestLog = end($propertyBag->logs); + $this->assertRegExp('/setBillingCountry input warnings.+\nNot ISO 3166-1.+\nGiven input matched a country name.*?\n.*munged to: "GB"/', $latestLog); + $this->assertEquals('GB', $propertyBag->getBillingCountry()); + + // Invalid country ID + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry(-1); + $this->assertCount(1, $propertyBag->logs); + $latestLog = end($propertyBag->logs); + $this->assertRegExp('/setBillingCountry input warnings.+\nExpected string\nGiven input looked like it could be a country ID but did not.*?\n.*munged to: ""/', $latestLog); + $this->assertEquals('', $propertyBag->getBillingCountry()); + + // Valid country ID + $propertyBag = new PropertyBag(); + $propertyBag->setBillingCountry(1154); /* should be New Zealand */ + $this->assertCount(1, $propertyBag->logs); + $latestLog = end($propertyBag->logs); + $this->assertRegExp('/setBillingCountry input warnings.+\nExpected string\nGiven input matched a country ID.*?\n.*munged to: "NZ"/', $latestLog); + $this->assertEquals('NZ', $propertyBag->getBillingCountry()); + } + /** * Test generic getter, setter methods. * From e965b6ff0a8d0657e736e5b2b59ddb0fb943326c Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Wed, 9 Nov 2022 09:19:51 +0000 Subject: [PATCH 2/2] PropertyBag::setBillingCountry - Change from log warning to deprecation messsage --- Civi/Payment/PropertyBag.php | 24 ++++++++++++------- .../phpunit/Civi/Payment/PropertyBagTest.php | 6 +++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php index c190258a145c..e82be81b2364 100644 --- a/Civi/Payment/PropertyBag.php +++ b/Civi/Payment/PropertyBag.php @@ -91,6 +91,15 @@ class PropertyBag implements \ArrayAccess { */ public $logs = []; + /** + * For unit tests only. Set to the name of a function, e.g. setBillingCountry + * to suppress calling CRM_Core_Error::deprecatedWarning which will break tests. + * Useful when a test is testing THAT a deprecatedWarning is thrown. + * + * @var string + */ + public string $ignoreDeprecatedWarningsInFunction = ''; + /** * @var bool * Temporary, internal variable to help ease transition to PropertyBag. @@ -698,7 +707,12 @@ public function setBillingCountry($input, $label = 'default') { if ($warnings) { $warnings[] = "Input: " . json_encode($input) . " was munged to: " . json_encode($munged); - $this->logWarning(__FUNCTION__ . " input warnings (may be errors in future):\n" . implode("\n", $warnings)); + $warnings = "PropertyBag::setBillingCountry input warnings (may be errors in future):\n" . implode("\n", $warnings); + $this->logs[] = $warnings; + // Emit a deprecatedWarning except in the case that we're testing this function. + if (__FUNCTION__ !== $this->ignoreDeprecatedWarningsInFunction) { + CRM_Core_Error::deprecatedWarning($warnings); + } } return $this->set('billingCountry', $label, $munged); @@ -1257,12 +1271,4 @@ public function setCustomProperty($prop, $value, $label = 'default') { $this->props[$label][$prop] = $value; } - /** - * For unit tests only. - */ - protected function logWarning($message) { - $this->logs[] = $message; - \Civi::log()->warning($message); - } - } diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php index e26a466b99d7..0602ca5e5e10 100644 --- a/tests/phpunit/Civi/Payment/PropertyBagTest.php +++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php @@ -463,6 +463,7 @@ public function testBillingCountry() { // Many sorts of nothingness foreach ([NULL, 0, FALSE] as $bad) { $propertyBag = new PropertyBag(); + $propertyBag->ignoreDeprecatedWarningsInFunction = 'setBillingCountry'; $propertyBag->setBillingCountry($bad); $this->assertCount(1, $propertyBag->logs); $latestLog = end($propertyBag->logs); @@ -472,6 +473,7 @@ public function testBillingCountry() { // '' special case $propertyBag = new PropertyBag(); + $propertyBag->ignoreDeprecatedWarningsInFunction = 'setBillingCountry'; $propertyBag->setBillingCountry(''); $this->assertCount(1, $propertyBag->logs); $latestLog = end($propertyBag->logs); @@ -480,6 +482,7 @@ public function testBillingCountry() { // Invalid country name $propertyBag = new PropertyBag(); + $propertyBag->ignoreDeprecatedWarningsInFunction = 'setBillingCountry'; $propertyBag->setBillingCountry('UnitedKing'); $this->assertCount(1, $propertyBag->logs); $latestLog = end($propertyBag->logs); @@ -488,6 +491,7 @@ public function testBillingCountry() { // Valid country name $propertyBag = new PropertyBag(); + $propertyBag->ignoreDeprecatedWarningsInFunction = 'setBillingCountry'; $propertyBag->setBillingCountry('United Kingdom'); $this->assertCount(1, $propertyBag->logs); $latestLog = end($propertyBag->logs); @@ -496,6 +500,7 @@ public function testBillingCountry() { // Invalid country ID $propertyBag = new PropertyBag(); + $propertyBag->ignoreDeprecatedWarningsInFunction = 'setBillingCountry'; $propertyBag->setBillingCountry(-1); $this->assertCount(1, $propertyBag->logs); $latestLog = end($propertyBag->logs); @@ -504,6 +509,7 @@ public function testBillingCountry() { // Valid country ID $propertyBag = new PropertyBag(); + $propertyBag->ignoreDeprecatedWarningsInFunction = 'setBillingCountry'; $propertyBag->setBillingCountry(1154); /* should be New Zealand */ $this->assertCount(1, $propertyBag->logs); $latestLog = end($propertyBag->logs);