Support hooks for MembershipType entity #11908
Conversation
CRM/Member/BAO/MembershipType.php
Outdated
| @@ -128,6 +139,20 @@ public static function add(&$params, $ids = array()) { | |||
| if ($id) { | |||
| self::updateAllPriceFieldValue($id, $params); | |||
| } | |||
|
|
|||
| if (!empty($params['custom']) && | |||
There was a problem hiding this comment.
@mattwire this should already be supported in the api layer in 5.0 see
civicrm/civicrm-dev-docs@b0bf2ba
We don't need to add via bao layer
CRM/Member/BAO/MembershipType.php
Outdated
| if (empty($params['id'])) { | ||
| $params['id'] = CRM_Utils_Array::value('membershipType', $ids); | ||
| } | ||
| $id = CRM_Utils_Array::value('id', $params); |
There was a problem hiding this comment.
I think the tidiest way of expressing this part is the Phone BAO
$hook = empty($params['id']) ? 'create' : 'edit';
CRM_Utils_Hook::pre($hook, 'Phone', CRM_Utils_Array::value('id', $params), $params);
There was a problem hiding this comment.
Agreed, updated to match that format
CRM/Member/BAO/MembershipType.php
Outdated
| */ | ||
| public static function add(&$params, $ids = array()) { | ||
| $id = CRM_Utils_Array::value('id', $params, CRM_Utils_Array::value('membershipType', $ids)); | ||
| if (empty($params['id'])) { | ||
| $params['id'] = CRM_Utils_Array::value('membershipType', $ids); |
There was a problem hiding this comment.
should we add a deprecation notice here?
CRM/Core/SelectValues.php
Outdated
| @@ -223,6 +223,7 @@ public static function customGroupExtends() { | |||
| 'ContributionRecur' => ts('Recurring Contributions'), | |||
| 'Group' => ts('Groups'), | |||
| 'Membership' => ts('Memberships'), | |||
| 'MembershipType' => ts('Membership Types'), | |||
There was a problem hiding this comment.
@eileenmcnaughton The problem here is that you need to set 3 parameters (label, value, name) in cg_extend_objects but you cannot do this via the UI as name is not exposed. So you are either limited to doing the DB change via an extension or editing the DB directly. It may also be possible to do it via the API but it seems a lot less straightforward than just adding it here which allows the user to directly select "Membership Types" as a custom data entity when creating custom fields.
And we now have support in core for displaying those custom fields via this PR
There was a problem hiding this comment.
@mattwire right - I think the intent IS that additional entities would be supported by adding values to that option group in an extension.
CRM/Core/BAO/CustomGroup.php
Outdated
| @@ -1813,6 +1813,9 @@ public static function mapTableName($table) { | |||
| case 'Membership': | |||
| return 'civicrm_membership'; | |||
|
|
|||
| case 'MembershipType': | |||
There was a problem hiding this comment.
@eileenmcnaughton See comment above
|
@mattwire the api already supports the custom data & we should switch to calling the api rather than doubling up on the BAO. The hook part we should add in the BAO create, as you have done. I'm in 2 minds about the ->find() addition because it imposes a performance cost on every save - regardless of whether hooks are in play. OTOH the hook itself can fetch extra data if required. (our code is inconsistent on this & as a low-traffic entity the performance is less of an issue but I think the precedent of leaving extra DB calls to the hook fn is better) |
d3936f7 to
acfc2f2
Compare
|
@eileenmcnaughton I've removed the ->find() per your comments. It's not required so I'll not add it here. |
|
@mattwire I would merge this PR if it were just the BAO changes - I'm not sold on those 2 custom data changes as part of this |
acfc2f2 to
56eaab9
Compare
56eaab9 to
000e4e8
Compare
|
@eileenmcnaughton The PR is now just the BAO changes to support hooks for MembershipType. Assuming it passes tests would you mind having another look? |
|
unrelated fails |
|
Agree this just adds hooks & a little cleanup - merging |
Overview
Basic changes required to support pre/post hooks on MembershipType entity (see #11794).
Before
After