Add pre() and post() hooks for ufgroup entity #18995
Conversation
|
Can one of the admins verify this patch? |
|
(Standard links)
|
|
add to whitelist |
|
@lucky091588 can you please fix this style issue |
|
@lucky091588 it's failing because it needs rebasing: See https://docs.civicrm.org/dev/en/latest/tools/git/#rebasing It's a pain but just adding more commits won't fix it. You could also start fresh which might be easier than fighting with git since git usually wins. If you get stuck try posting at https://chat.civicrm.org (maybe in the |
|
I thank everyone for the helpful comments. This is still WIP, and @twomice is helping me get it ready for review. |
0b97519 to
8aeec5f
Compare
Looking at civicrm#18995 I was struck by the delete hook being non-standard. In general we don't load from the DB just to pass to the hook as the hook can do that itself. However, when I looked at other hooks I found that they were passing out nullArray - this is a legacy method that precedes being on a php version that supported default params when passing by reference. It's further known to introduce intermittent hard to debug issues. This adds the new hook and also adds standardisation to the other hooks for pre+delete. I've left the create one for now but GroupContact is a good example of something close to what we want to standardise on there
CRM/Core/BAO/UFGroup.php
Outdated
| $group = new CRM_Core_DAO_UFGroup(); | ||
| $group->id = $id; | ||
| $group->fetch(); | ||
| $params = $group->toArray(); |
There was a problem hiding this comment.
We don't normally load from the db just to pass out to hooks -the db load can be done in the hook if it wants it.
I took a look at the other delete hooks & realised they are a bit of a dogs breakfast so I put up this PR to make the laster parameter optional & clean them up a bit
There was a problem hiding this comment.
@lucky091588 Eileen's commit #19059 means we'll need to rebase this PR and probably resolve some git merge conflicts. Let's talk about that offline.
| * Class CRM_Core_BAO_UFGroupTest | ||
| * @group headless | ||
| */ | ||
| class CRM_Core_BAO_UFGroupTest extends CiviUnitTestCase { |
There was a problem hiding this comment.
Loving these tests!
CRM/Core/BAO/UFGroup.php
Outdated
| @@ -1451,10 +1458,19 @@ public static function add(&$params, $ids = []) { | |||
| if (!empty($params['group_type']) && is_array($params['group_type'])) { | |||
| $params['group_type'] = implode(',', $params['group_type']); | |||
| } | |||
|
|
|||
| $ufGroupID = CRM_Utils_Array::value('ufgroup', $ids, CRM_Utils_Array::value('id', $params)); | |||
There was a problem hiding this comment.
If possible can you squash this down - like we do on GroupContact
$hook = empty($params['id']) ? 'create' : 'edit';
CRM_Utils_Hook::pre($hook, 'GroupContact', ($params['id'] ?? NULL) , $params);
Note it's OK not to check the $ids because that param can be deprecated - I'll put up a PR for that
bb0f436 to
aa72bcb
Compare
|
This PR is ready for review. |
CRM/Core/BAO/UFGroup.php
Outdated
| $ufGroup = new CRM_Core_DAO_UFGroup(); | ||
| $ufGroup->copyValues($params); | ||
|
|
||
| $ufGroupID = CRM_Utils_Array::value('ufgroup', $ids, CRM_Utils_Array::value('id', $params)); | ||
| if (!$ufGroupID && empty($params['name'])) { | ||
| if (empty($params['name'])) { |
There was a problem hiding this comment.
I don't think we can drop ufGroupID from this check. I think you removed it in order to use the preferred shorter syntax for the hook - but it looks like the variable is still needed
|
It's looking very close now |
c3485f1 to
214a370
Compare
|
I think this is ready for review and I myself would recommend committing, but Joinery is sponsoring this work so conflict of interest exists. @eileenmcnaughton could you comment/review? |
|
This looks good to me and the code looks nice n clean and can confirm the hook tests are being executed by Jenkins merging |
|
@lucky091588 congrats on your first PR being merged can you please create a PR to add in information into here https://github.com/civicrm/civicrm-core/blob/master/contributor-key.yml for yourself please. |
Overview
This is to add ufgroup support for pre() and post() hooks
Gitlab Issue: https://lab.civicrm.org/dev/core/-/issues/2199
Before
ufgroup was not supported with pre() and post() hooks.
After
ufgroup will be supported with pre() and post() hooks.