Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Switch OptionGroup BAO to use new centralized logic to make name from title #22654

Merged
merged 1 commit into from
Feb 3, 2022
Merged

[REF] Switch OptionGroup BAO to use new centralized logic to make name from title #22654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions CRM/Core/BAO/CustomField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a material change it seems @colemanw what prompted this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in order to take advantage of the auto-name-generation, I needed to switch from direct DAO insert to using the WriteRecord function. The name gets derived from the title, which previously was just the label of the custom field. I wanted it to be more descriptive. It doesn't matter too much because neither name nor title are ever shown in the UI anywhere for auto-generated custom groups, but for techie looking at the sql table and trying to make sense of what's in it, I thought this more precise label would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw are they shown at the list of option groups i.e. at /admin/options?reset=1 also as part of a drop down when creating a new custom field right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you kind of have to go looking for it, but it will show in the UI in those places. Again, because you have to go looking, I think the name of the custom group + the name of the custom field is an improvement over just the field name.

// 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) {
Expand Down
4 changes: 2 additions & 2 deletions CRM/Core/BAO/OptionGroup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'])) {
Expand Down
20 changes: 15 additions & 5 deletions CRM/Core/DAO.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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')) {
Expand All @@ -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());
Expand All @@ -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;
}
Expand Down
6 changes: 4 additions & 2 deletions CRM/Utils/String.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
7 changes: 5 additions & 2 deletions api/v3/OptionGroup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/api/v3/CustomFieldTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]);
Expand Down
1 change: 1 addition & 0 deletions tests/phpunit/api/v3/SyntaxConformanceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ public static function toBeSkipped_custom_data_creatable($sequential = FALSE) {
'Profile',
'Payment',
'Order',
'OptionGroup',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR triggered a bizarre test failure, why it ever worked before I don't know, but no, attaching custom data to the OptionGroup entity makes no sense.

'MailingGroup',
'Logging',
];
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/api/v4/Action/CreateCustomValueTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down