DAO - Centralize logic to derive unique name from label

This function checks db indices to enforce uniqueness of "name" column
when creating a new record via the `DAO::writeRecord()` method.

diff --git a/CRM/Contact/BAO/SavedSearch.php b/CRM/Contact/BAO/SavedSearch.php
index c9141eaa56c32e93185f168bc5af361ef20b3789..cd45672464f19273cb53554a8e4ac5e6869c0dfb 100644 (file)
--- a/CRM/Contact/BAO/SavedSearch.php
+++ b/CRM/Contact/BAO/SavedSearch.php
@@ -277,19 +277,6 @@ WHERE  $where";
    * @return \CRM_Contact_DAO_SavedSearch
    */
   public static function create(&$params) {
-    // Auto-create unique name from label if supplied
-    if (empty($params['id']) && empty($params['name']) && !empty($params['label'])) {
-      $name = CRM_Utils_String::munge($params['label']);
-      $existing = Civi\Api4\SavedSearch::get(FALSE)
-        ->addWhere('name', 'LIKE', $name . '%')
-        ->addSelect('name')
-        ->execute()->column('name');
-      $suffix = '';
-      while (in_array($name . $suffix, $existing)) {
-        $suffix = '_' . (1 + str_replace('_', '', $suffix));
-      }
-      $params['name'] = $name . $suffix;
-    }
     $loggedInContactID = CRM_Core_Session::getLoggedInContactID();
     if ($loggedInContactID) {
       if (empty($params['id'])) {

diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
index 2d28ec7ffe1eec684b261beb0485391694c2e344..140e929cda82801e31e18d422f8101fa33d9e2d8 100644 (file)
--- a/CRM/Core/DAO.php
+++ b/CRM/Core/DAO.php
@@ -918,10 +918,14 @@ class CRM_Core_DAO extends DB_DataObject {
     $entityName = CRM_Core_DAO_AllCoreTables::getBriefName($className);
 
     \CRM_Utils_Hook::pre($op, $entityName, $record['id'] ?? NULL, $record);
-    $instance = new $className();
+    $fields = static::getSupportedFields();
+    $instance = new static();
     // Ensure fields exist before attempting to write to them
-    $values = array_intersect_key($record, self::getSupportedFields());
+    $values = array_intersect_key($record, $fields);
     $instance->copyValues($values);
+    if (empty($values['id']) && array_key_exists('name', $fields) && empty($values['name']) && !empty($values['label'])) {
+      $instance->makeNameFromLabel();
+    }
     $instance->save();
 
     if (!empty($record['custom']) && is_array($record['custom'])) {
@@ -3279,4 +3283,44 @@ SELECT contact_id
     return static::$_paths ?? [];
   }
 
+  /**
+   * When creating a record without a supplied name,
+   * create a unique, clean name derived from the label.
+   *
+   * Note: this function does nothing unless a unique index exists for "name" column.
+   */
+  private function makeNameFromLabel() {
+    $indexNameWith = NULL;
+    // Look for a unique index which includes the "name" field
+    if (method_exists($this, 'indices')) {
+      foreach ($this->indices(FALSE) as $index) {
+        if (!empty($index['unique']) && in_array('name', $index['field'], TRUE)) {
+          $indexNameWith = $index['field'];
+        }
+      }
+    }
+    if (!$indexNameWith) {
+      // No unique index on "name", do nothing
+      return;
+    }
+    $name = CRM_Utils_String::munge($this->label, '_', 252);
+
+    // Find existing records with the same name
+    $sql = new CRM_Utils_SQL_Select($this::getTableName());
+    $sql->select(['id', 'name']);
+    $sql->where('name LIKE @name', ['@name' => $name . '%']);
+    // Include all fields that are part of the index
+    foreach (array_diff($indexNameWith, ['name']) as $field) {
+      $sql->where("`$field` = @val", ['@val' => $this->$field]);
+    }
+    $query = $sql->toSQL();
+    $existing = self::executeQuery($query)->fetchMap('id', 'name');
+    $dupes = 0;
+    $suffix = '';
+    while (in_array($name . $suffix, $existing)) {
+      $suffix = '_' . (++$dupes);
+    }
+    $this->name = $name . $suffix;
+  }
+
 }

diff --git a/ext/search_kit/search_kit.php b/ext/search_kit/search_kit.php
index 24a0e76058af4593fda561d6b738a9b39ddce263..b3163c327adae39e1922c958e98f299f7c8ecd94 100644 (file)
--- a/ext/search_kit/search_kit.php
+++ b/ext/search_kit/search_kit.php
@@ -83,15 +83,6 @@ function search_kit_civicrm_entityTypes(&$entityTypes) {
  * Implements hook_civicrm_pre().
  */
 function search_kit_civicrm_pre($op, $entity, $id, &$params) {
-  // Supply default name/label when creating new SearchDisplay
-  if ($entity === 'SearchDisplay' && $op === 'create') {
-    if (empty($params['label'])) {
-      $params['label'] = $params['name'];
-    }
-    elseif (empty($params['name'])) {
-      $params['name'] = \CRM_Utils_String::munge($params['label']);
-    }
-  }
   // When deleting a saved search, also delete the displays
   // This would happen anyway in sql because of the ON DELETE CASCADE foreign key,
   // But this ensures that pre and post hooks are called

diff --git a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php
index eebceeaee1591c6cdbbc10f531305a9ce6c38b49..d56366c6ade9865d269a65fe46b81cf4d81779e6 100644 (file)
--- a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php
+++ b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchDisplayTest.php
@@ -1,6 +1,7 @@
 <?php
 namespace api\v4\SearchDisplay;
 
+use Civi\Api4\SavedSearch;
 use Civi\Api4\SearchDisplay;
 use Civi\Test\HeadlessInterface;
 use Civi\Test\TransactionalInterface;
@@ -50,4 +51,44 @@ class SearchDisplayTest extends \PHPUnit\Framework\TestCase implements HeadlessI
     $this->assertNull($display['saved_search_id']);
   }
 
+  public function testAutoFormatName() {
+    // Create 3 saved searches; they should all get unique names
+    $savedSearch0 = SavedSearch::create(FALSE)
+      ->addValue('label', 'My test search')
+      ->execute()->first();
+    $savedSearch1 = SavedSearch::create(FALSE)
+      ->addValue('label', 'My test search')
+      ->execute()->first();
+    $savedSearch2 = SavedSearch::create(FALSE)
+      ->addValue('label', 'My test search')
+      ->execute()->first();
+    // Name will be created from munged label
+    $this->assertEquals('My_test_search', $savedSearch0['name']);
+    // Name will have _1, _2, etc. appended to ensure it's unique
+    $this->assertEquals('My_test_search_1', $savedSearch1['name']);
+    $this->assertEquals('My_test_search_2', $savedSearch2['name']);
+
+    $display0 = SearchDisplay::create()
+      ->addValue('saved_search_id', $savedSearch0['id'])
+      ->addValue('label', 'My test display')
+      ->addValue('type', 'table')
+      ->execute()->first();
+    $display1 = SearchDisplay::create()
+      ->addValue('saved_search_id', $savedSearch0['id'])
+      ->addValue('label', 'My test display')
+      ->addValue('type', 'table')
+      ->execute()->first();
+    $display2 = SearchDisplay::create()
+      ->addValue('saved_search_id', $savedSearch1['id'])
+      ->addValue('label', 'My test display')
+      ->addValue('type', 'table')
+      ->execute()->first();
+    // Name will be created from munged label
+    $this->assertEquals('My_test_display', $display0['name']);
+    // Name will have _1 appended to ensure it's unique to savedSearch0
+    $this->assertEquals('My_test_display_1', $display1['name']);
+    // This is for a different saved search so doesn't need a number appended
+    $this->assertEquals('My_test_display', $display2['name']);
+  }
+
 }