Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#3977 Handle dodgier calls to setBillingCountry in property bag. #24927

Merged
merged 2 commits into from
Nov 11, 2022
Merged

dev/core#3977 Handle dodgier calls to setBillingCountry in property bag. #24927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea9fb73
Handle dodgier calls to setBillingCountry in property bag re issue co…
artfulrobot Nov 8, 2022
e965b6f
PropertyBag::setBillingCountry - Change from log warning to deprecati…
artfulrobot Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 68 additions & 5 deletions Civi/Payment/PropertyBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use InvalidArgumentException;
use CRM_Core_Error;
use CRM_Core_PseudoConstant;
use Civi\Api4\Country;

/**
* @class
Expand Down Expand Up @@ -83,6 +84,21 @@ class PropertyBag implements \ArrayAccess {
'isNotifyProcessorOnCancelRecur' => TRUE,
];

/**
* For unit tests only.
*
* @var array
*/
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
Expand Down Expand Up @@ -646,13 +662,60 @@ 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 (!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) {
// 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.";
}
}
}
return $this->set('billingCountry', $label, (string) $input);

if ($warnings) {
$warnings[] = "Input: " . json_encode($input) . " was munged to: " . json_encode($munged);
$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);
}

/**
Expand Down
71 changes: 70 additions & 1 deletion tests/phpunit/Civi/Payment/PropertyBagTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'], ['', '']], []],
Expand All @@ -448,6 +447,76 @@ 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->ignoreDeprecatedWarningsInFunction = 'setBillingCountry';
$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->ignoreDeprecatedWarningsInFunction = 'setBillingCountry';
$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->ignoreDeprecatedWarningsInFunction = 'setBillingCountry';
$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->ignoreDeprecatedWarningsInFunction = 'setBillingCountry';
$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->ignoreDeprecatedWarningsInFunction = 'setBillingCountry';
$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->ignoreDeprecatedWarningsInFunction = 'setBillingCountry';
$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.
*
Expand Down