From 30208fab037dfe47c014dec4cec4429b0aeaf1d8 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 25 Jan 2018 16:09:23 +1300 Subject: [PATCH] CRM-21707 use serialisation metadata from api basic_create_fallback. In order to add a unit test I enabled the same in-DAO handling on the Group BAO create (since core api don't use the fallback). It allowed me to remove cruft and to remove comments about removing cruft. With this in place a custom entity (in an extension) could define group_type SEPARATOR_BOOKEND and that field would accept an array of valid names that map to group_types and the api would handle the serialisation. api v4 supports this so good prep --- CRM/Contact/BAO/Group.php | 33 ++++++---------------- CRM/Contact/DAO/Group.php | 3 +- CRM/Core/DAO.php | 12 +++++++- CRM/Core/Error.php | 2 +- api/v3/utils.php | 2 +- tests/phpunit/api/v3/GroupTest.php | 45 ++++++++++++++++++++++++++---- xml/schema/Contact/Group.xml | 1 + 7 files changed, 65 insertions(+), 33 deletions(-) diff --git a/CRM/Contact/BAO/Group.php b/CRM/Contact/BAO/Group.php index 9aa7704fb6..05e78491fc 100644 --- a/CRM/Contact/BAO/Group.php +++ b/CRM/Contact/BAO/Group.php @@ -356,16 +356,13 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group { $params['name'] = CRM_Utils_String::titleToVar($params['title']); } + if (!empty($params['parents'])) { + $params['parents'] = CRM_Utils_Array::convertCheckboxFormatToArray((array) $params['parents']); + } + // convert params if array type if (isset($params['group_type'])) { - if (is_array($params['group_type'])) { - $params['group_type'] = CRM_Core_DAO::VALUE_SEPARATOR . implode(CRM_Core_DAO::VALUE_SEPARATOR, - CRM_Utils_Array::convertCheckboxFormatToArray($params['group_type']) - ) . CRM_Core_DAO::VALUE_SEPARATOR; - } - else { - $params['group_type'] = CRM_Core_DAO::VALUE_SEPARATOR . $params['group_type'] . CRM_Core_DAO::VALUE_SEPARATOR; - } + $params['group_type'] = CRM_Utils_Array::convertCheckboxFormatToArray((array) $params['group_type']); } else { $params['group_type'] = NULL; @@ -391,17 +388,8 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group { } } $group = new CRM_Contact_BAO_Group(); - $group->copyValues($params); - //@todo very hacky fix for the fact this function wants to receive 'parents' as an array further down but - // needs it as a separated string for the DB. Preferred approaches are having the copyParams or save fn - // use metadata to translate the array to the appropriate DB type or altering the param in the api layer, - // or at least altering the param in same section as 'group_type' rather than repeating here. However, further down - // we need the $params one to be in it's original form & we are not sure what test coverage we have on that - if (isset($group->parents) && is_array($group->parents)) { - $group->parents = CRM_Core_DAO::VALUE_SEPARATOR . implode(CRM_Core_DAO::VALUE_SEPARATOR, - array_keys($group->parents) - ) . CRM_Core_DAO::VALUE_SEPARATOR; - } + $group->copyValues($params, TRUE); + if (empty($params['id']) && !$nameParam ) { @@ -437,14 +425,11 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group { ) { // if no parent present and the group doesn't already have any parents, // make sure site group goes as parent - $params['parents'] = array($domainGroupID => 1); - } - elseif (array_key_exists('parents', $params) && !is_array($params['parents'])) { - $params['parents'] = array($params['parents'] => 1); + $params['parents'] = array($domainGroupID); } if (!empty($params['parents'])) { - foreach ($params['parents'] as $parentId => $dnc) { + foreach ($params['parents'] as $parentId) { if ($parentId && !CRM_Contact_BAO_GroupNesting::isParentChild($parentId, $group->id)) { CRM_Contact_BAO_GroupNesting::add($parentId, $group->id); } diff --git a/CRM/Contact/DAO/Group.php b/CRM/Contact/DAO/Group.php index 49fa0363d0..e7625923b6 100644 --- a/CRM/Contact/DAO/Group.php +++ b/CRM/Contact/DAO/Group.php @@ -6,7 +6,7 @@ * * Generated from xml/schema/CRM/Contact/Group.xml * DO NOT EDIT. Generated by CRM_Core_CodeGen - * (GenCodeChecksum:848664fcd390bd178325b0d991c8e947) + * (GenCodeChecksum:d0b8ad59d0e80070e103ca1db922cfd5) */ /** @@ -381,6 +381,7 @@ class CRM_Contact_DAO_Group extends CRM_Core_DAO { 'entity' => 'Group', 'bao' => 'CRM_Contact_BAO_Group', 'localizable' => 0, + 'serialize' => self::SERIALIZE_COMMA, ], 'children' => [ 'name' => 'children', diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index d5eab412c1..5ab5d76d52 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -610,11 +610,17 @@ class CRM_Core_DAO extends DB_DataObject { * * @param array $params * (reference ) associative array of name/value pairs. + * @param bool $serializeArrays + * Should arrays that are passed in be serialised according to the metadata. + * Eventually this should be always true / gone, but in the interests of caution + * it is being grandfathered in. In general an array is not valid on the DAO + * but there may be instances where this function is called & then some handling + * takes place on the would-be array. * * @return bool * Did we copy all null values into the object */ - public function copyValues(&$params) { + public function copyValues(&$params, $serializeArrays = FALSE) { $fields = $this->fields(); $allNull = TRUE; foreach ($fields as $name => $value) { @@ -636,6 +642,10 @@ class CRM_Core_DAO extends DB_DataObject { if ($pValue === '') { $this->$dbName = 'null'; } + elseif ($serializeArrays && is_array($pValue) && !empty($value['serialize'])) { + $this->$dbName = CRM_Core_DAO::serializeField($pValue, $value['serialize']); + $allNull = FALSE; + } else { $this->$dbName = $pValue; $allNull = FALSE; diff --git a/CRM/Core/Error.php b/CRM/Core/Error.php index df1ab58a61..88bb83505d 100644 --- a/CRM/Core/Error.php +++ b/CRM/Core/Error.php @@ -106,7 +106,7 @@ class CRM_Core_Error extends PEAR_ErrorStack { * @param bool $throwPEAR_Error * @param string $stackClass * - * @return object + * @return CRM_Core_Error */ public static function &singleton($package = NULL, $msgCallback = FALSE, $contextCallback = FALSE, $throwPEAR_Error = FALSE, $stackClass = 'PEAR_ErrorStack') { if (self::$_singleton === NULL) { diff --git a/api/v3/utils.php b/api/v3/utils.php index 3fed9d32a9..9dadc310df 100644 --- a/api/v3/utils.php +++ b/api/v3/utils.php @@ -1346,7 +1346,7 @@ function _civicrm_api3_basic_create_fallback($bao_name, &$params) { CRM_Utils_Hook::pre($hook, $entityName, CRM_Utils_Array::value('id', $params), $params); $instance = new $dao_name(); - $instance->copyValues($params); + $instance->copyValues($params, TRUE); $instance->save(); CRM_Utils_Hook::post($hook, $entityName, $instance->id, $instance); diff --git a/tests/phpunit/api/v3/GroupTest.php b/tests/phpunit/api/v3/GroupTest.php index f03f05a25e..2ea630bbcb 100644 --- a/tests/phpunit/api/v3/GroupTest.php +++ b/tests/phpunit/api/v3/GroupTest.php @@ -150,7 +150,7 @@ class api_v3_GroupTest extends CiviUnitTestCase { //check for empty parent 'parents' => "", 'visibility' => 'Public Pages', - 'group_type' => array(1, 2), + 'group_type' => [1, 2], ); $result = $this->callAPISuccess('Group', 'create', $params); @@ -178,12 +178,47 @@ class api_v3_GroupTest extends CiviUnitTestCase { 'title' => 'Test Group 2', 'group_type' => 2, 'parents' => $result['id'], + 'sequential' => 1, ) ); - $result = $this->callAPISuccess('Group', 'create', $params); - $group = $result["values"][$result['id']]; - $this->assertEquals($group['group_type'], array($params['group_type'])); - $this->assertEquals($group['parents'], $params['parents']); + $group2 = $this->callAPISuccess('Group', 'create', $params)['values'][0]; + + $this->assertEquals($group2['group_type'], array($params['group_type'])); + $this->assertEquals($params['parents'], $group2['parents']); + + // Test array format for parents. + $params = array_merge($params, array( + 'name' => 'Test Group 3', + 'title' => 'Test Group 3', + 'parents' => [$result['id'], $group2['id']], + ) + ); + $group3 = $this->callAPISuccess('Group', 'create', $params)['values'][0]; + $parents = $this->callAPISuccess('Group', 'getvalue', ['return' => 'parents', 'id' => $group3['id']]); + + $this->assertAPIArrayComparison("{$result['id']},{$group2['id']}", $parents); + + $groupNesting = $this->callAPISuccess('GroupNesting', 'get', ['child_group_id' => $group3['id']]); + // 2 Group nesting entries - one for direct parent & one for grandparent. + $this->assertEquals(2, $groupNesting['count']); + $this->groupDelete($group2['id']); + $this->groupDelete($group3['id']); + } + + /** + * Test that an array of valid values works for group_type field. + */ + public function testGroupTypeWithPseudoconstantArray() { + $params = [ + 'name' => 'Test Group 2', + 'title' => 'Test Group 2', + 'group_type' => ['Mailing List', 'Access Control'], + 'sequential' => 1, + ]; + $group = $this->callAPISuccess('Group', 'create', $params); + $groupType = $this->callAPISuccess('Group', 'getvalue', ['return' => 'group_type', 'id' => $group['id']]); + + $this->assertAPIArrayComparison([2, 1], $groupType); } public function testGetNonExistingGroup() { diff --git a/xml/schema/Contact/Group.xml b/xml/schema/Contact/Group.xml index 68ad6eb3f4..28c7d96e76 100644 --- a/xml/schema/Contact/Group.xml +++ b/xml/schema/Contact/Group.xml @@ -149,6 +149,7 @@ Group Parents IDs of the parent(s) 2.1 + COMMA children -- 2.25.1