From 5727486c1b257da5d00261481699fe91f25d2189 Mon Sep 17 00:00:00 2001
From: Coleman Watts <coleman@civicrm.org>
Date: Wed, 9 Feb 2022 11:35:53 -0500
Subject: [PATCH] dev/core#3063 APIv3 - Fix numeric option matching

Before: Option matching was skipped for all FK fields if a numeric value was given
After: Only skipped for `campaign_id` field, if positive integer given

The optimization was overly broad and had unintended side-effects
---
 api/v3/utils.php                           |  8 ++--
 tests/phpunit/api/v3/FinancialTypeTest.php | 44 ++++++++++++++++++++++
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/api/v3/utils.php b/api/v3/utils.php
index 1bbc77ef45..a7ce83160e 100644
--- a/api/v3/utils.php
+++ b/api/v3/utils.php
@@ -2077,10 +2077,10 @@ function _civicrm_api3_validate_integer(&$params, $fieldName, &$fieldInfo, $enti
       }
     }
     if (
-    (!empty($fieldInfo['pseudoconstant']) || !empty($fieldInfo['options']) || $fieldName === 'campaign_id')
-     // if it is already numeric AND it is an FK field we don't need to validate because
-     // sql will do that for us on insert (this also saves a big lookup)
-     && (!is_numeric($fieldValue) || empty($fieldInfo['FKClassName']))
+      !empty($fieldInfo['pseudoconstant']) ||
+      !empty($fieldInfo['options']) ||
+      // Special case for campaign_id which is no longer a pseudoconstant
+      ($fieldName === 'campaign_id' && !CRM_Utils_Rule::positiveInteger($fieldValue))
     ) {
       $additional_lookup_params = [];
       if (strtolower($entity) === 'address' && $fieldName == 'state_province_id') {
diff --git a/tests/phpunit/api/v3/FinancialTypeTest.php b/tests/phpunit/api/v3/FinancialTypeTest.php
index 4b9a653123..44805af8d0 100644
--- a/tests/phpunit/api/v3/FinancialTypeTest.php
+++ b/tests/phpunit/api/v3/FinancialTypeTest.php
@@ -114,4 +114,48 @@ class api_v3_FinancialTypeTest extends CiviUnitTestCase {
     $this->assertEquals('INC', $result['account_type_code'], 'Financial account created is not an income account.');
   }
 
+  public function testMatchFinancialTypeOptions() {
+    // Just a string name, should be simple to match on
+    $nonNumericOption = $this->callAPISuccess('FinancialType', 'create', [
+      'name' => 'StringName',
+    ])['id'];
+    // A numeric name, but a number that won't match any existing id
+    $numericOptionUnique = $this->callAPISuccess('FinancialType', 'create', [
+      'name' => '999',
+    ])['id'];
+    // Here's the kicker, a numeric name that matches an existing id!
+    $numericOptionMatchingExistingId = $this->callAPISuccess('FinancialType', 'create', [
+      'name' => $nonNumericOption,
+    ])['id'];
+    $cid = $this->individualCreate();
+
+    // Create a contribution matching non-numeric name
+    $contributionWithNonNumericType = $this->callAPISuccess('Contribution', 'create', [
+      'financial_type_id' => 'StringName',
+      'total_amount' => 100,
+      'contact_id' => $cid,
+      'sequential' => TRUE,
+    ]);
+    $this->assertEquals($nonNumericOption, $contributionWithNonNumericType['values'][0]['financial_type_id']);
+
+    // Create a contribution matching unique numeric name
+    $contributionWithUniqueNumericType = $this->callAPISuccess('Contribution', 'create', [
+      'financial_type_id' => '999',
+      'total_amount' => 100,
+      'contact_id' => $cid,
+      'sequential' => TRUE,
+    ]);
+    $this->assertEquals($numericOptionUnique, $contributionWithUniqueNumericType['values'][0]['financial_type_id']);
+
+    // Create a contribution matching the id of the non-numeric option, which is ambiguously the name of another option
+    $contributionWithAmbiguousNumericType = $this->callAPISuccess('Contribution', 'create', [
+      'financial_type_id' => "$nonNumericOption",
+      'total_amount' => 100,
+      'contact_id' => $cid,
+      'sequential' => TRUE,
+    ]);
+    // The id should have taken priority over matching by name
+    $this->assertEquals($nonNumericOption, $contributionWithAmbiguousNumericType['values'][0]['financial_type_id']);
+  }
+
 }
-- 
2.25.1