"install_groupid" and "install_matching" issues

4ndygu / civicrm_osdi

An Extension for importing from and exporting to OSDI endpoints from CiviCRM

Other

4 stars 4 forks source link

"install_groupid" and "install_matching" issues #9

Closed MegaphoneJon closed 6 years ago

MegaphoneJon commented 6 years ago

Hi Andy,

I know you're done, but you said you'd be available for bugfixes. Let me know if you think this isn't something you'll get to.

In the code I noticed a couple of things about these two functions.

They're called by the "enable" and "postInstall" hooks. If I understand them correctly, they should just run once, on hook_civicrm_install.
They don't have docblocks, so I'm actually not 100% sure. I see that install groupid creates the custom field group and field; I haven't investigated what install_matching does, but if you could add a docblock, it would help future maintainers of the code.
hook_civicrm_uninstall should have functions that reverse these functions (e.g. delete the custom field and group).

Thanks, Jon

4ndygu commented 6 years ago

ill work on cleaning these up this week!

4ndygu commented 6 years ago

Do you feel like someone who disables and enables the application should have any functionality trigger?

MegaphoneJon commented 6 years ago

No - disable/enable don't typically change the installed data. A good user story for disable/enable is "I think there's a bug in this extension that's causing a problem elsewhere in CiviCRM, let me turn it off for a minute to test". They would expect to be able to re-enable and resume where they left off.

Someone who uninstalls would ideally like all traces of the extension removed. This is particularly important because the extension won't reinstall cleanly in its current state.

4ndygu commented 6 years ago

makes sense to me - i merged some code which will delete everything installed, adds code blocks to the functions and the install functions, and moves all this behavior into the install hook.

https://github.com/4ndygu/civicrm_osdi/pull/10/files

MegaphoneJon commented 6 years ago

This looks good! With one exception of a typo, which I've submitted a PR for in #11.