CustomGroup - Ensure 'name' is always unique

[colemanw]

Overview

Stricter enforcement of CustomGroup name uniqueness.

Before

Previously, unique 'name' was indexed in tandem with 'extends'.

After

Now it is ensured to be unique unconditionally.

Technical Details

Note that at the form layer, unconditionally unique names have always been enforced, so in practice, every custom group name should already be unique.

civicrm-core/CRM/Custom/Form/Group.php

Lines 95 to 102 in aca7613

	$name = CRM_Utils_String::munge($title, '_', 64);
	$query = 'select count(*) from civicrm_custom_group where ( name like %1) and id != %2';
	$grpCnt = CRM_Core_DAO::singleValueQuery($query, [
	1 => [$name, 'String'],
	2 => [(int) $self->_id, 'Integer'],
	]);
	if ($grpCnt) {
	$errors['title'] = ts('Custom group \'%1\' already exists in Database.', [1 => $title]);

However this was never enforced at the BAO or API layer, so this PR includes an upgrade script to uniquify duplicate names.

Comments

The upgrade sql is simplistic, it will update any pair of duplicates found, but doesn't cover the possibility of triplets or quadruplets. But I think that's extremely unlikely - due to the enforcement that was happening at the form layer there really shouldn't be any duplicates at all; so the upgrade sql is there as a safeguard rather than a necessity.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

I wonder if we need some messaging - on the one hand we are changing name which could be used to access fields. On the other it would have been flakey.... I could imaging names being used in civicrm_managed - which would be ... flakey.

We could possibly add a pre-message if any will be altered? Which would probably be rare but might alert them to any odd potential

[colemanw]

@eileenmcnaughton I've added the pre-upgrade check.

[colemanw]

Tests are green and I really think this needs to be merged asap. As @eileenmcnaughton points out, the existence of non-unique names would be flaky in managed entities... but it just occurred to me that they would be even flakier in APIv4, which relies on name uniqueness for getting and setting all custom data!
@demeritcowboy @seamuslee001

[demeritcowboy]

It runs ok just two suggestions:

1. The message gets hidden because of the styling of the bullets above and looks like boilerplate footer text which the brain just skips. If it had its own bullet it would be more visible.

2. If they do read it, the average person will think it means the label will be changed. Maybe: "You have custom field groups with duplicate internal names in your system. They will automatically be renamed. If you have custom code that refers to these names then you will need to update that code to match the new names."

[colemanw]

Thanks for testing this @demeritcowboy. I've updated the message text, but kept the markup as-is because it's consistent with just about all other markup for pre-upgrade messages. I agree that making these messages easier to read is something to work on, but I see it as a separate issue.