From 8293f7c2c397db650b65448b10d09f6542ba4ba1 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Wed, 23 Sep 2020 14:06:46 +1200
Subject: [PATCH] dev/core#2057 Remove select query that never or almost never
 finds something.

When it comes to quey efficiency it's good to do an extra select if it saves an update/insert
but not if it never does. To test the theory that this 'never does' I put
removing this select through the entire test suite. No tests failed, meaning
it was unnecessary 100% of the time. Adding the catch ensures
it will still succeed if it the row exists but we have moved our query conservation
from doing an extraneous select in 99.9% of cases to an extraneous update in .1% of cases

https://lab.civicrm.org/dev/core/-/issues/2057
---
 CRM/Activity/BAO/ActivityContact.php         | 15 ++++++++++++---
 tests/phpunit/api/v3/ActivityContactTest.php | 16 +++++++++++++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/CRM/Activity/BAO/ActivityContact.php b/CRM/Activity/BAO/ActivityContact.php
index 527e664da5..6efa32ab9d 100644
--- a/CRM/Activity/BAO/ActivityContact.php
+++ b/CRM/Activity/BAO/ActivityContact.php
@@ -37,13 +37,22 @@ class CRM_Activity_BAO_ActivityContact extends CRM_Activity_DAO_ActivityContact
    *   activity_contact object
    */
   public static function create($params) {
+    $errorScope = CRM_Core_TemporaryErrorScope::useException();
     $activityContact = new CRM_Activity_DAO_ActivityContact();
-
     $activityContact->copyValues($params);
-    if (!$activityContact->find(TRUE)) {
+    try {
       return $activityContact->save();
     }
-    return $activityContact;
+    catch (PEAR_Exception $e) {
+      // This check used to be done first, creating an extra query before each insert.
+      // However, in none of the core tests was this ever called with values that already
+      // existed, meaning that this line would never or almost never be hit.
+      // hence it's better to save the select query here.
+      if ($activityContact->find(TRUE)) {
+        return $activityContact;
+      }
+      throw $e;
+    }
   }
 
   /**
diff --git a/tests/phpunit/api/v3/ActivityContactTest.php b/tests/phpunit/api/v3/ActivityContactTest.php
index 00884061ec..73d690f32f 100644
--- a/tests/phpunit/api/v3/ActivityContactTest.php
+++ b/tests/phpunit/api/v3/ActivityContactTest.php
@@ -40,21 +40,28 @@ class api_v3_ActivityContactTest extends CiviUnitTestCase {
 
   /**
    * @param int $version
+   *
    * @dataProvider versionThreeAndFour
+   * @throws \CRM_Core_Exception
    */
   public function testCreateActivityContact($version) {
     $this->_apiversion = $version;
 
-    $result = $this->callAPIAndDocument('activity_contact', 'create', $this->_params, __FUNCTION__, __FILE__);
+    $result = $this->callAPIAndDocument('ActivityContact', 'create', $this->_params, __FUNCTION__, __FILE__);
     $this->assertEquals(1, $result['count']);
     $this->assertNotNull($result['values'][$result['id']]['id']);
 
+    $result = $this->callAPIAndDocument('ActivityContact', 'create', $this->_params, __FUNCTION__, __FILE__);
+    $this->assertEquals(1, $result['count']);
+
     $this->callAPISuccess('activity_contact', 'delete', ['id' => $result['id']]);
   }
 
   /**
    * @param int $version
+   *
    * @dataProvider versionThreeAndFour
+   * @throws \CRM_Core_Exception
    */
   public function testDeleteActivityContact($version) {
     $this->_apiversion = $version;
@@ -71,7 +78,9 @@ class api_v3_ActivityContactTest extends CiviUnitTestCase {
 
   /**
    * @param int $version
+   *
    * @dataProvider versionThreeAndFour
+   * @throws \CRM_Core_Exception
    */
   public function testGetActivitiesByContact($version) {
     $this->_apiversion = $version;
@@ -80,7 +89,9 @@ class api_v3_ActivityContactTest extends CiviUnitTestCase {
 
   /**
    * @param int $version
+   *
    * @dataProvider versionThreeAndFour
+   * @throws \CRM_Core_Exception
    */
   public function testGetActivitiesByActivity($version) {
     $this->_apiversion = $version;
@@ -89,8 +100,11 @@ class api_v3_ActivityContactTest extends CiviUnitTestCase {
 
   /**
    * Test civicrm_activity_contact_get with empty params.
+   *
    * @param int $version
+   *
    * @dataProvider versionThreeAndFour
+   * @throws \CRM_Core_Exception
    */
   public function testGetEmptyParams($version) {
     $this->_apiversion = $version;
-- 
2.25.1