Merge pull request #21211 from colemanw/onDeletePrimary

[REF] Move Email, Address, etc. is_primary handling on delete to a hook (#2757) 

CRM/Contact/BAO/Contact.php

@@ -14,7 +14,7 @@
  * @package CRM
  * @copyright CiviCRM LLC https://civicrm.org/licensing
  */
-class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact {
+class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact implements Civi\Test\HookInterface {
 
   /**
    * SQL function used to format the phone_numeric field via trigger.
@@ -3511,53 +3511,31 @@ LEFT JOIN civicrm_address ON ( civicrm_address.contact_id = civicrm_contact.id )
   }
 
   /**
-   * Delete a contact-related object that has an 'is_primary' field.
-   *
-   * Ensures that is_primary gets assigned to another object if available
-   * Also calls pre/post hooks
-   *
-   * @param string $type
-   * @param int $id
-   *
-   * @return bool
+   * Event fired after modifying any entity.
+   * @param \Civi\Core\Event\PostEvent $event
    */
-  public static function deleteObjectWithPrimary($type, $id) {
-    if (!$id || !is_numeric($id)) {
-      return FALSE;
-    }
-    $daoName = "CRM_Core_DAO_$type";
-    $obj = new $daoName();
-    $obj->id = $id;
-    $obj->find();
-
-    if ($obj->fetch()) {
-      CRM_Utils_Hook::pre('delete', $type, $id);
-      $contactId = $obj->contact_id;
-      $obj->delete();
-    }
-    else {
-      return FALSE;
-    }
-    // is_primary is only relavent if this field belongs to a contact
-    if ($contactId) {
-      $dao = new $daoName();
-      $dao->contact_id = $contactId;
+  public static function on_hook_civicrm_post(\Civi\Core\Event\PostEvent $event) {
+    // Handle deleting a related entity with is_primary
+    $hasPrimary = ['Address', 'Email', 'IM', 'OpenID', 'Phone'];
+    if (
+      $event->action === 'delete' && $event->id &&
+      in_array($event->entity, $hasPrimary) &&
+      !empty($event->object->is_primary) &&
+      !empty($event->object->contact_id)
+    ) {
+      $daoClass = CRM_Core_DAO_AllCoreTables::getFullName($event->entity);
+      $dao = new $daoClass();
+      $dao->contact_id = $event->object->contact_id;
       $dao->is_primary = 1;
       // Pick another record to be primary (if one isn't already)
       if (!$dao->find(TRUE)) {
         $dao->is_primary = 0;
-        $dao->find();
-        if ($dao->fetch()) {
-          $dao->is_primary = 1;
-          $dao->save();
-          if ($type === 'Email') {
-            CRM_Core_BAO_UFMatch::updateUFName($dao->contact_id);
-          }
+        if ($dao->find(TRUE)) {
+          $baoClass = CRM_Core_DAO_AllCoreTables::getBAOClassName($daoClass);
+          $baoClass::writeRecord(['id' => $dao->id, 'is_primary' => 1]);
         }
       }
     }
-    CRM_Utils_Hook::post('delete', $type, $id, $obj);
-    return TRUE;
   }
 
   /**

CRM/Core/BAO/Address.php

@@ -1210,12 +1210,14 @@ SELECT is_primary,
   /**
    * Call common delete function.
    *
-   * @param int $id
+   * @see \CRM_Contact_BAO_Contact::on_hook_civicrm_post
    *
+   * @param int $id
+   * @deprecated
    * @return bool
    */
   public static function del($id) {
-    return CRM_Contact_BAO_Contact::deleteObjectWithPrimary('Address', $id);
+    return (bool) self::deleteRecord(['id' => $id]);
   }
 
   /**

CRM/Core/BAO/Email.php

@@ -20,7 +20,7 @@ use Civi\Api4\Email;
 /**
  * This class contains functions for email handling.
  */
-class CRM_Core_BAO_Email extends CRM_Core_DAO_Email {
+class CRM_Core_BAO_Email extends CRM_Core_DAO_Email implements Civi\Test\HookInterface {
   use CRM_Contact_AccessTrait;
 
   /**
@@ -80,15 +80,21 @@ WHERE  contact_id = {$params['contact_id']}
       self::updateContactName($contactId, $address);
     }
 
-    if ($email->is_primary) {
-      // update the UF user email if that has changed
-      CRM_Core_BAO_UFMatch::updateUFName($email->contact_id);
-    }
-
     CRM_Utils_Hook::post($hook, 'Email', $email->id, $email);
     return $email;
   }
 
+  /**
+   * Event fired after modifying an Email.
+   * @param \Civi\Core\Event\PostEvent $event
+   */
+  public static function self_hook_civicrm_post(\Civi\Core\Event\PostEvent $event) {
+    if ($event->action !== 'delete' && !empty($event->object->is_primary) && !empty($event->object->contact_id)) {
+      // update the UF user email if that has changed
+      CRM_Core_BAO_UFMatch::updateUFName($event->object->contact_id);
+    }
+  }
+
   /**
    * Takes an associative array and adds email.
    *
@@ -342,12 +348,14 @@ AND    reset_date IS NULL
   /**
    * Call common delete function.
    *
-   * @param int $id
+   * @see \CRM_Contact_BAO_Contact::on_hook_civicrm_post
    *
+   * @param int $id
+   * @deprecated
    * @return bool
    */
   public static function del($id) {
-    return CRM_Contact_BAO_Contact::deleteObjectWithPrimary('Email', $id);
+    return (bool) self::deleteRecord(['id' => $id]);
   }
 
   /**

CRM/Core/BAO/IM.php

@@ -158,12 +158,14 @@ ORDER BY cim.is_primary DESC, im_id ASC ";
   /**
    * Call common delete function.
    *
-   * @param int $id
+   * @see \CRM_Contact_BAO_Contact::on_hook_civicrm_post
    *
+   * @param int $id
+   * @deprecated
    * @return bool
    */
   public static function del($id) {
-    return CRM_Contact_BAO_Contact::deleteObjectWithPrimary('IM', $id);
+    return (bool) self::deleteRecord(['id' => $id]);
   }
 
 }

CRM/Core/BAO/OpenID.php

@@ -121,12 +121,14 @@ ORDER BY
   /**
    * Call common delete function.
    *
-   * @param int $id
+   * @see \CRM_Contact_BAO_Contact::on_hook_civicrm_post
    *
+   * @param int $id
+   * @deprecated
    * @return bool
    */
   public static function del($id) {
-    return CRM_Contact_BAO_Contact::deleteObjectWithPrimary('OpenID', $id);
+    return (bool) self::deleteRecord(['id' => $id]);
   }
 
 }

CRM/Core/BAO/Phone.php

@@ -234,12 +234,14 @@ ORDER BY ph.is_primary DESC, phone_id ASC ";
   /**
    * Call common delete function.
    *
-   * @param int $id
+   * @see \CRM_Contact_BAO_Contact::on_hook_civicrm_post
    *
+   * @param int $id
+   * @deprecated
    * @return bool
    */
   public static function del($id) {
-    return CRM_Contact_BAO_Contact::deleteObjectWithPrimary('Phone', $id);
+    return (bool) self::deleteRecord(['id' => $id]);
   }
 
 }

CRM/Core/DAO.php

@@ -962,9 +962,14 @@ class CRM_Core_DAO extends DB_DataObject {
     CRM_Utils_Hook::pre('delete', $entityName, $record['id'], $record);
     $instance = new $className();
     $instance->id = $record['id'];
-    if (!$instance->delete()) {
+    // Load complete object for the sake of hook_civicrm_post, below
+    $instance->find(TRUE);
+    if (!$instance || !$instance->delete()) {
       throw new CRM_Core_Exception("Could not delete {$entityName} id {$record['id']}");
     }
+    // For other operations this hook is passed an incomplete object and hook listeners can load if needed.
+    // But that's not possible with delete because it's gone from the database by the time this hook is called.
+    // So in this case the object has been pre-loaded so hook listeners have access to the complete record.
     CRM_Utils_Hook::post('delete', $entityName, $record['id'], $instance);
 
     return $instance;

tests/phpunit/api/v4/Entity/AddressTest.php

+<?php
+
+/*
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC. All rights reserved.                        |
+ |                                                                    |
+ | This work is published under the GNU AGPLv3 license with some      |
+ | permitted exceptions and without any warranty. For full license    |
+ | and copyright information, see https://civicrm.org/licensing       |
+ +--------------------------------------------------------------------+
+ */
+
+/**
+ *
+ * @package CRM
+ * @copyright CiviCRM LLC https://civicrm.org/licensing
+ */
+
+namespace api\v4\Entity;
+
+use api\v4\UnitTestCase;
+use Civi\Api4\Address;
+use Civi\Api4\Contact;
+use Civi\Test\TransactionalInterface;
+
+/**
+ * Test Address functionality
+ *
+ * @group headless
+ */
+class AddressTest extends UnitTestCase implements TransactionalInterface {
+
+  /**
+   * Check that 2 addresses for the same contact can't both be primary
+   */
+  public function testPrimary() {
+    $cid = Contact::create(FALSE)->addValue('first_name', uniqid())->execute()->single()['id'];
+
+    $a1 = Address::create(FALSE)
+      ->addValue('is_primary', TRUE)
+      ->addValue('contact_id', $cid)
+      ->addValue('location_type_id', 1)
+      ->addValue('city', 'Somewhere')
+      ->execute();
+
+    $a2 = Address::create(FALSE)
+      ->addValue('is_primary', TRUE)
+      ->addValue('contact_id', $cid)
+      ->addValue('location_type_id', 2)
+      ->addValue('city', 'Elsewhere')
+      ->execute();
+
+    $addresses = Address::get(FALSE)
+      ->addWhere('contact_id', '=', $cid)
+      ->addOrderBy('id')
+      ->execute();
+
+    $this->assertFalse($addresses[0]['is_primary']);
+    $this->assertTrue($addresses[1]['is_primary']);
+  }
+
+}