Switch OptionGroup BAO to use new centralized logic to make name from title

Switches APIv4 to use writeRecords for the OptionGroup entity;
keeps v3 the same for backward-compatability.

diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php
index ee68902d0b7c969ffff21cb37fee0a47e34f4edf..21547af73ce7997efe826ebe05cae3bb41eb3c2f 100644 (file)
--- a/CRM/Core/BAO/CustomField.php
+++ b/CRM/Core/BAO/CustomField.php
@@ -2028,14 +2028,13 @@ WHERE  id IN ( %1, %2 )
         )
       ) {
         // first create an option group for this custom group
-        $optionGroup = new CRM_Core_DAO_OptionGroup();
-        $optionGroup->name = "{$params['column_name']}_" . date('YmdHis');
-        $optionGroup->title = $params['label'];
-        $optionGroup->is_active = 1;
-        // Don't set reserved as it's not a built-in option group and may be useful for other custom fields.
-        $optionGroup->is_reserved = 0;
-        $optionGroup->data_type = $dataType;
-        $optionGroup->save();
+        $customGroupTitle = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomGroup', $params['custom_group_id'], 'title');
+        $optionGroup = CRM_Core_DAO_OptionGroup::writeRecord([
+          'title' => "$customGroupTitle :: {$params['label']}",
+          // Don't set reserved as it's not a built-in option group and may be useful for other custom fields.
+          'is_reserved' => 0,
+          'data_type' => $dataType,
+        ]);
         $params['option_group_id'] = $optionGroup->id;
         if (!empty($params['option_value']) && is_array($params['option_value'])) {
           foreach ($params['option_value'] as $k => $v) {

diff --git a/CRM/Core/BAO/OptionGroup.php b/CRM/Core/BAO/OptionGroup.php
index 420537a16666894785fe26dbf576585716d2ba5d..cce3128251e9918c3821b5d169400512c9c50a57 100644 (file)
--- a/CRM/Core/BAO/OptionGroup.php
+++ b/CRM/Core/BAO/OptionGroup.php
@@ -59,8 +59,8 @@ class CRM_Core_BAO_OptionGroup extends CRM_Core_DAO_OptionGroup implements \Civi
    * @param array $ids
    *   Reference array contains the id.
    *
-   *
-   * @return object
+   * @deprecated
+   * @return CRM_Core_DAO_OptionGroup
    */
   public static function add(&$params, $ids = []) {
     if (empty($params['id']) && !empty($ids['optionGroup'])) {

diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
index 2f9809a6c69e257a0acb09b4b49a3276e5c7018b..3ce3ebdc03d23d8da5b01e163b5b208e4a128c71 100644 (file)
--- a/CRM/Core/DAO.php
+++ b/CRM/Core/DAO.php
@@ -923,8 +923,8 @@ class CRM_Core_DAO extends DB_DataObject {
     // Ensure fields exist before attempting to write to them
     $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();
+    if (empty($values['id']) && array_key_exists('name', $fields) && empty($values['name'])) {
+      $instance->makeNameFromLabel(!empty($fields['name']['required']));
     }
     $instance->save();
 
@@ -3290,8 +3290,10 @@ SELECT contact_id
    * create a unique, clean name derived from the label.
    *
    * Note: this function does nothing unless a unique index exists for "name" column.
+   *
+   * @var bool $isRequired
    */
-  private function makeNameFromLabel() {
+  private function makeNameFromLabel(bool $isRequired): void {
     $indexNameWith = NULL;
     // Look for a unique index which includes the "name" field
     if (method_exists($this, 'indices')) {
@@ -3305,7 +3307,14 @@ SELECT contact_id
       // No unique index on "name", do nothing
       return;
     }
-    $name = CRM_Utils_String::munge($this->label, '_', 252);
+    $label = $this->label ?? $this->title ?? NULL;
+    if (!$label && $label !== '0' && !$isRequired) {
+      // No label supplied and name not required, do nothing
+      return;
+    }
+    $maxLen = static::getSupportedFields()['name']['maxlength'] ?? 255;
+    // Strip unsafe characters and trim to max length (-3 characters leaves room for a unique suffix)
+    $name = CRM_Utils_String::munge($label, '_', $maxLen - 3);
 
     // Find existing records with the same name
     $sql = new CRM_Utils_SQL_Select($this::getTableName());
@@ -3319,8 +3328,9 @@ SELECT contact_id
     $existing = self::executeQuery($query)->fetchMap('id', 'name');
     $dupes = 0;
     $suffix = '';
+    // Add unique suffix if existing records have the same name
     while (in_array($name . $suffix, $existing)) {
-      $suffix = '_' . (++$dupes);
+      $suffix = '_' . ++$dupes;
     }
     $this->name = $name . $suffix;
   }

diff --git a/CRM/Utils/String.php b/CRM/Utils/String.php
index 5ee79d831c24d622da13a9e617cd63a1d92216a6..4292ed1a61d880c0d9b3be4ca1f3552cde51a35f 100644 (file)
--- a/CRM/Utils/String.php
+++ b/CRM/Utils/String.php
@@ -43,12 +43,14 @@ class CRM_Utils_String {
   public static function titleToVar($title, $maxLength = 31) {
     $variable = self::munge($title, '_', $maxLength);
 
+    // FIXME: nothing below this line makes sense. The above call to self::munge will always
+    // return a safe string of the correct length, so why are we now checking if it's a safe
+    // string of the correct length?
     if (CRM_Utils_Rule::title($variable, $maxLength)) {
       return $variable;
     }
 
-    // if longer than the maxLength lets just return a substr of the
-    // md5 to prevent errors downstream
+    // FIXME: When would this ever be reachable?
     return substr(md5($title), 0, $maxLength);
   }
 

diff --git a/api/v3/OptionGroup.php b/api/v3/OptionGroup.php
index 794d7441cad1b41379c844b19fe6adcd52eca4f5..5025480d4f0a6f8907cec3d593add1bb42439c86 100644 (file)
--- a/api/v3/OptionGroup.php
+++ b/api/v3/OptionGroup.php
@@ -37,9 +37,12 @@ function civicrm_api3_option_group_get($params) {
  * @return array
  */
 function civicrm_api3_option_group_create($params) {
-  $result = _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params, 'OptionGroup');
+  // Use deprecated BAO method in APIv3 for legacy support. APIv4 uses new writeRecords method.
+  $bao = CRM_Core_BAO_OptionGroup::add($params);
   civicrm_api('option_value', 'getfields', ['version' => 3, 'cache_clear' => 1]);
-  return $result;
+  $values = [];
+  _civicrm_api3_object_to_array($bao, $values[$bao->id]);
+  return civicrm_api3_create_success($values, $params, 'OptionGroup', 'create', $bao);
 }
 
 /**

diff --git a/tests/phpunit/api/v3/CustomFieldTest.php b/tests/phpunit/api/v3/CustomFieldTest.php
index 0fc13cea2b03218cde632b3d126efe971c4c6fdb..f1253bfc6cba39b7fc49dc8efcea9c5f083c615b 100644 (file)
--- a/tests/phpunit/api/v3/CustomFieldTest.php
+++ b/tests/phpunit/api/v3/CustomFieldTest.php
@@ -203,7 +203,7 @@ class api_v3_CustomFieldTest extends CiviUnitTestCase {
     $optionGroup = $this->callAPISuccess('option_group', 'getsingle', [
       'id' => $optionGroupID,
     ]);
-    $this->assertEquals('Country', $optionGroup['title']);
+    $this->assertEquals('select_test_group :: Country', $optionGroup['title']);
     $optionValueCount = $this->callAPISuccess('option_value', 'getcount', [
       'option_group_id' => $optionGroupID,
     ]);

diff --git a/tests/phpunit/api/v3/SyntaxConformanceTest.php b/tests/phpunit/api/v3/SyntaxConformanceTest.php
index 98ec86bc84a130e807bf6197905cffb3f9ca857b..6dbaff805c63fa85512a32c10ca3411ac26eb2f0 100644 (file)
--- a/tests/phpunit/api/v3/SyntaxConformanceTest.php
+++ b/tests/phpunit/api/v3/SyntaxConformanceTest.php
@@ -423,6 +423,7 @@ class api_v3_SyntaxConformanceTest extends CiviUnitTestCase {
       'Profile',
       'Payment',
       'Order',
+      'OptionGroup',
       'MailingGroup',
       'Logging',
     ];

diff --git a/tests/phpunit/api/v4/Action/CreateCustomValueTest.php b/tests/phpunit/api/v4/Action/CreateCustomValueTest.php
index 10278cc84abc47b91c04dfe5a9f5129bb9f08555..03529f4038bb295d84beb6e48565feafeb59ed97 100644 (file)
--- a/tests/phpunit/api/v4/Action/CreateCustomValueTest.php
+++ b/tests/phpunit/api/v4/Action/CreateCustomValueTest.php
@@ -59,7 +59,7 @@ class CreateCustomValueTest extends BaseCustomValueTest {
       ->execute()
       ->first();
 
-    $this->assertEquals('Color', $optionGroup['title']);
+    $this->assertEquals('MyContactFields :: Color', $optionGroup['title']);
 
     $createdOptionValues = OptionValue::get(FALSE)
       ->addWhere('option_group_id', '=', $optionGroupId)