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.

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 - Use CRM_Contact_BAO_ContactType::basicTypes() instead of hardcoded lists #22389

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 6, 2022

Overview

Uses a function to return contact types wherever they're needed.

Before

Codebase littered with hardcoded arrays like ['Individual', 'Organization', 'Household'].

After

Uses function which returns that array.

Technical Details

This makes code more flexible/forgiving if new contact types are added in the future, or if existing contact types are disabled.

Also passes TRUE as the 3rd param to in_array() which is best-practice to avoid type coercion.

@civibot
Copy link

civibot bot commented Jan 6, 2022

(Standard links)

@braders
Copy link
Contributor

braders commented Jan 6, 2022

Looks good with a quick test on the test site. I've just noticed one more instance of ['Household', 'Individual', 'Organization'] in the _civicrm_api3_custom_format_params function which probably wants updating at the same time. Otherwise looks good.

I've also noticed templates/CRM/UF/Page/ProfileTemplates.tpl has the types hardcoded in the template. Possibly out of scope for this PR, but you might want to do a follow-up.

There is also the contactOptions field in CRM/Admin/Form/Options.php which should probably be built from the helper function (albiet with 'Multiple Contact Merge' added). The number of places this is hardcoded has definitely reduced after this change though.

…coded lists

Makes code more flexible/forgiving if new contact types are added in the future,
or if existing contact types are disabled.
@colemanw
Copy link
Member Author

colemanw commented Jan 6, 2022

Thanks @braders I've updated _civicrm_api3_custom_format_params. The others are a little trickier so I'll keep this PR simple enough to be mergeable.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jan 6, 2022
@eileenmcnaughton
Copy link
Contributor

I went through this PR & all the changes make sense - a couple of unused params removed too

Merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants