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

… title

CRM/Core/BAO/CustomField.php

@@ -2028,14 +2028,13 @@ protected static function prepareCreate($params) {
         )
       ) {
         // 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) {

CRM/Core/BAO/OptionGroup.php

@@ -59,8 +59,8 @@ public static function setIsActive($id, $is_active) {
    * @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'])) {

CRM/Core/DAO.php

@@ -923,8 +923,8 @@ public static function writeRecord(array $record): CRM_Core_DAO {
     // 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 @@ public static function getEntityPaths() {
    * 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 @@ private function makeNameFromLabel() {
       // 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 @@ private function makeNameFromLabel() {
     $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;
   }

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);
   }
 

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);
 }
 
 /**

tests/phpunit/api/v3/CustomFieldTest.php

@@ -203,7 +203,7 @@ public function testCustomFieldCreateWithEmptyOptionGroup(): void {
     $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,
     ]);

tests/phpunit/api/v4/Action/CreateCustomValueTest.php

@@ -59,7 +59,7 @@ public function testGetWithCustomData() {
       ->execute()
       ->first();
 
-    $this->assertEquals('Color', $optionGroup['title']);
+    $this->assertEquals('MyContactFields :: Color', $optionGroup['title']);
 
     $createdOptionValues = OptionValue::get(FALSE)
       ->addWhere('option_group_id', '=', $optionGroupId)