CRM-20410 Reformat CRM_SMS_BAO_Provider to use standard create function style rather than the split #10135
Conversation
seamuslee001
commented
Apr 11, 2017
•
seamuslee001
commented
- CRM-20410: Refactor SMS provider BAO to have proper create function
seamuslee001
force-pushed
the
CRM-20410
branch
2 times, most recently
from
5f86e4f
to
e645f61
Compare
CRM/SMS/BAO/Provider.php
Outdated
| $dao->save(); | ||
| } | ||
| public static function create($params, $ids = array()) { | ||
| $id = CRM_Utils_Array::value('id', $ids, CRM_Utils_Array::value('id', $params)); |
There was a problem hiding this comment.
the $ids array is legacy - just use params for an new fn
There was a problem hiding this comment.
seems like as per https://github.com/civicrm/civicrm-core/blob/master/api/v3/utils.php#L1281 it expects a 2nd param which is ids
There was a problem hiding this comment.
I think it handles rather than expects $ids
| @@ -64,12 +64,12 @@ public function testCreateAndUpdateProvider() { | |||
| 'is_active' => 1, | |||
| 'api_type' => 1, | |||
| ); | |||
| CRM_SMS_BAO_Provider::saveRecord($values); | |||
| CRM_SMS_BAO_Provider::create($values, array()); | |||
There was a problem hiding this comment.
I tend to think it only matters to test the api - the BAO is tested in the process & the api is the recommended way to call it. BAO can change, but api is the one we commit to
| $provider->id = $id; | ||
| $provider->find(TRUE); | ||
| } | ||
| if ($id) { |
There was a problem hiding this comment.
so, in theory all of this could be in the extension & be removed from here
There was a problem hiding this comment.
up to L116 but yes agree with that if needed
| } | ||
| elseif ($this->_action & CRM_Core_Action::ADD) { | ||
| CRM_SMS_BAO_Provider::saveRecord($recData); | ||
| if ($this->_action && (CRM_Core_Action::UPDATE || CRM_Core_Action::ADD)) { |
There was a problem hiding this comment.
$ids is not required - set $params['id'] instead
There was a problem hiding this comment.
is it not? it seems like it is needed as per basic_create function
There was a problem hiding this comment.
no - only $params is needed - $ids is being phased out
There was a problem hiding this comment.
(maybe you should comment whereever you got that from...)
There was a problem hiding this comment.
CRM/SMS/Form/Provider.php
Outdated
| else { | ||
| $ids = array(); | ||
| } | ||
| CRM_SMS_BAO_Provider::create($recData, $ids); |
|
@seamuslee001 this looks good - only question is whether we should put the domain part in an extension. I'm trying to decide if putting in the extension instead of here will make it easier later or it's a 'might as well merge & when @nganivet does his big change it can be adjusted then.... |
|
I would say merge ... I always dream I can do so many things, but the hash reality of customer commitments kicks in and I have to postpone. Might be a while before I have time for this big refactor, although it's nice to see these pieces fall in place. |
|
Well at least @seamuslee001 effort adding an api will make it easier |
…on style rather than the split Fix posthook
Minor fix
|
Jenkins re test this please |
1 similar comment
|
Jenkins re test this please |
|
Oh - I thought we'd merged this when I blamed it for breakage :-) |
|
test this please |
