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] Follow up fix to fixing note entity tables in views #646

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

seamuslee001
Copy link
Contributor

@civibot
Copy link

civibot bot commented Jul 7, 2021

(Standard links)

@civibot civibot bot added the 7.x-master label Jul 7, 2021
@seamuslee001 seamuslee001 force-pushed the fix_note_entity_table branch from 1d21775 to d659777 Compare July 7, 2021 07:35
Comment on lines 1162 to 1163
$noteEntityTables = civicrm_api3('OptionValue', 'get', ['option_group_id' => 'note_used_for'])['values'];
foreach (array_values(CRM_Utils_Array::collect('value', $noteEntityTables)) as $entityTable) {
Copy link
Member

Choose a reason for hiding this comment

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

I think both the before and after versions are incorrect because they access lower-level data instead of doing a pseudoconstant lookup and letting that layer worry about where the data is coming from. If it had been doing this from the beginning, we wouldn't need any change here:

Suggested change
$noteEntityTables = civicrm_api3('OptionValue', 'get', ['option_group_id' => 'note_used_for'])['values'];
foreach (array_values(CRM_Utils_Array::collect('value', $noteEntityTables)) as $entityTable) {
$noteEntities = civicrm_api3('Note', 'getoptions', ['field' => "entity_table"])['values'];
foreach (array_keys($noteEntities) as $entityTable) {

@seamuslee001 seamuslee001 force-pushed the fix_note_entity_table branch from d659777 to c487a8b Compare July 7, 2021 22:08
@seamuslee001
Copy link
Contributor Author

good point @colemanw applied the patch here now

@colemanw colemanw merged commit f471e1a into civicrm:7.x-master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants