diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php index ee68902d0b7c..21547af73ce7 100644 --- a/CRM/Core/BAO/CustomField.php +++ b/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) { diff --git a/CRM/Core/BAO/OptionGroup.php b/CRM/Core/BAO/OptionGroup.php index 420537a16666..cce3128251e9 100644 --- a/CRM/Core/BAO/OptionGroup.php +++ b/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'])) { diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 2f9809a6c69e..3ce3ebdc03d2 100644 --- a/CRM/Core/DAO.php +++ b/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; } diff --git a/CRM/Utils/String.php b/CRM/Utils/String.php index 5ee79d831c24..4292ed1a61d8 100644 --- 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 794d7441cad1..5025480d4f0a 100644 --- 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 0fc13cea2b03..f1253bfc6cba 100644 --- a/tests/phpunit/api/v3/CustomFieldTest.php +++ b/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, ]); diff --git a/tests/phpunit/api/v3/SyntaxConformanceTest.php b/tests/phpunit/api/v3/SyntaxConformanceTest.php index 98ec86bc84a1..6dbaff805c63 100644 --- a/tests/phpunit/api/v3/SyntaxConformanceTest.php +++ b/tests/phpunit/api/v3/SyntaxConformanceTest.php @@ -423,6 +423,7 @@ public static function toBeSkipped_custom_data_creatable($sequential = FALSE) { '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 10278cc84abc..03529f4038bb 100644 --- a/tests/phpunit/api/v4/Action/CreateCustomValueTest.php +++ b/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)