-
-
Notifications
You must be signed in to change notification settings - Fork 824
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.
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
Ensure custom group name does not conflict with existing field #20694
Conversation
(Standard links)
|
} | ||
else { | ||
$group->name = CRM_Utils_String::munge($group->title, '_', 64); | ||
$group->name = CRM_Utils_String::munge($group->title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting the default max length is 63 so this will work with the current table definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this PR changes it from 64 down to 63 (which happens to be the default for that function) to accommodate the potential extra character inserted during validation.
} | ||
|
||
self::validateCustomGroupName($group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colemanw should this only be run on creation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right I see that now
if ($extendsDAO) { | ||
$fields = array_column($extendsDAO::fields(), 'name'); | ||
if (in_array($group->name, $fields)) { | ||
$group->name .= '0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colemanw what happens if you try to create a 2nd ActivityType Custom Group? i.e. one already has a 0 on it would that be an issue (this is probably quite the edge case but just thinking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior is that if you try to create a custom group with a duplicate name (or for that matter, a duplicate title) then you'll get a DB error due to the unique key constraints.
This PR doesn't alter that.
This seems fine to me merging |
Overview
Adds validation when creating a new custom group to prevent name conflicts with existing fields.
Technical Details
In APIv4, you access custom fields like
myCustomGroup.my_field
. That works as long as the custom group isn't named something like "contact_type" or "activity_type_id" which would lead to very confusing api calls with unpredictable results.Comments
Validates according to the entity being extended.