[REF] Improve APIv4 save functions #22403

colemanw · 2022-01-07T02:33:18Z

Overview

Refactors APIv4 create/update functions to make the code more flexible & maintainable.

Before

Dealing with oddball BAOs meant a bunch of hardcoded exceptions written into the APIv4 writeObjects function.

After

APIv4 writeObjects function refactored into smaller pieces.
Oddball handling moved into specific APIs.
Use @deprecated annotations in the BAO to decide whether to use generic writeRecords function.
Starts to centralize saving custom data rather than make each BAO responsible for it, which should make it easier to opt-in new entities to having custom data.

civibot · 2022-01-07T02:33:21Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2022-01-07T21:31:36Z

CRM/Campaign/BAO/Campaign.php

@@ -65,11 +65,6 @@ public static function create(&$params) {
      }
    }

-    //store custom data


I'ved checked this one & confirm it

eileenmcnaughton · 2022-01-07T21:31:41Z

CRM/Campaign/BAO/Survey.php

@@ -74,9 +74,6 @@ public static function create(&$params) {

    $dao = self::writeRecord($params);

-    if (!empty($params['custom']) && is_array($params['custom'])) {


I'ved checked this one & confirm it

eileenmcnaughton · 2022-01-07T23:59:05Z

CRM/Core/BAO/WordReplacement.php

   * @param array $params
   * @param int $id
-   *
   * @return array
   */
  public static function edit(&$params, &$id) {


I don' t think this function is ever called - we should go that step further & add a deprecation notice

We could go that step further & add a deprecation notice to the v3 function if it were to call something else itself - I think we could add in something like _civicrm_api3_basic_create that would call apiv4 & return those results & start deprecating create functions where we can really migrate off them

That's a longer-term goal I think.

eileenmcnaughton · 2022-01-08T00:36:16Z

@colemanw the last commit will mean that apiv3 updates custom twice in some cases - ie if it calls writeRecord on them & they are not one of the specified entities

Would this last commit pass on it's own? Can it be it's own PR?

civicrm-core/api/v3/utils.php

Lines 1340 to 1344 in 2d35bc7

    
           // If we have custom fields the BAO may have taken care of it or we may have to. 
        
           // $extendsMap provides a pretty good hard-coded list of BAOs that take care of the custom data. 
        
           if (isset($params['custom']) && empty(CRM_Core_BAO_CustomQuery::$extendsMap[$entity])) { 
        
             CRM_Core_BAO_CustomValueTable::store($params['custom'], CRM_Core_DAO_AllCoreTables::getTableForClass(CRM_Core_DAO_AllCoreTables::getFullName($entity)), $bao->id); 
        
           }

colemanw · 2022-01-08T19:28:22Z

@eileenmcnaughton of the 6 commits in this PR, the first 3 have been broken out & merged. I've broken the last one out to #22426 and included an APIv3 update in that PR. I've rebased this PR with the 2 remaining commits.

eileenmcnaughton · 2022-01-09T04:29:19Z

@colemanw I actually pulled out the group contact stuff to this PR #22419 - however I hit something that has me scratching my head. I created the SubscriptionHistory api for the test so I decided I should use that instead of the BAO method in the post hook - but when I do it runs and works but doesn't commit to the db - this feels like something I've seen before in extensions implementing post hooks so now we have a test scenario it's failing in it feels worth figuring our rather than bypassing it

(I think the general idea is that all crud should be by the v4 API - although classes do weird things like call add from create & such like)

eileenmcnaughton · 2022-01-09T04:31:30Z

Civi/Api4/Action/Address/AddressSaveTrait.php

@@ -47,14 +47,16 @@ trait AddressSaveTrait {
  /**
   * @inheritDoc
   */
-  protected function writeObjects(&$items) {
-    foreach ($items as &$item) {
+  protected function writeToDao(array $items) {


I'd like to bike shed this function name a little - I found it really confusing and we are not actually writing to the dao as it implies. We are 'saving' or perhaps 'writeToDatabase'

If 'save' is contaminated? maybe 'saveRecords' ?

Ok. Just to frame the issue, what's going on in this PR is that one function is getting split into two smaller functions. The function is writeObjects() which previously would:

format the params

call the appropriate BAO create/add/writeRecords function

Reformat and return the results

Now it does 1 and 3, with 2 being split off to this new sub-function. For compatibility reasons I don't think the outer writeObjects function should be renamed. So we need to pick a name for the inner sub-function. Something to indicate that it's called from writeObjects and is the piece that takes care of the actual writing (or rather, handing off to the BAO to do the writing). So I named it writeToDao.

@colemanw what about writeToDatabase - alternativelyperhap writeUsingDAO - except it would really be writeUsingBAOExceptIfThereIsn'tOne

Or even just ... write

The writeToDAO just doesn't seem to be what is actually being done - it's not being written to the DAO - and in some fantasy future world we could replace the DAO layer altogether.....

Yea, write sounds good.

…ions Instead of using a hardcoded list of BAO functions to call, with extra hardcoded params to e.g. the Contact & Address BAOs, this switches to picking the BAO's create/add only if it's not `@deprecated`, and falling back to WriteRecords. Also refactors the large `writeObjects` function into two smaller functions so individual action classes can more easily override them.

colemanw · 2022-01-10T21:21:02Z

@eileenmcnaughton ok this PR is now down to just one commit since you pulled the other one out into #22419 and I've renamed the function writeBikeToShed()

eileenmcnaughton · 2022-01-10T21:23:57Z

lol - looks OK to me - I'll go through a bit more carefully soon

eileenmcnaughton · 2022-01-13T00:57:32Z

OK I took another look & this still looks OK (with the group contact one still needing resolution) - test cover is good. Merging

civibot bot added the master label Jan 7, 2022

colemanw force-pushed the api4WriteRecords branch 2 times, most recently from 2065f6d to 6c41af3 Compare January 7, 2022 18:27

eileenmcnaughton reviewed Jan 7, 2022

View reviewed changes

colemanw force-pushed the api4WriteRecords branch from 6c41af3 to 34eea26 Compare January 8, 2022 19:26

eileenmcnaughton reviewed Jan 9, 2022

View reviewed changes

colemanw force-pushed the api4WriteRecords branch 2 times, most recently from b7c326a to 49054d6 Compare January 10, 2022 21:10

colemanw force-pushed the api4WriteRecords branch from 49054d6 to 5a44345 Compare January 10, 2022 21:18

eileenmcnaughton merged commit 4526b69 into civicrm:master Jan 13, 2022

eileenmcnaughton deleted the api4WriteRecords branch January 13, 2022 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Improve APIv4 save functions #22403

[REF] Improve APIv4 save functions #22403

colemanw commented Jan 7, 2022 •

edited

Loading

civibot bot commented Jan 7, 2022

eileenmcnaughton Jan 7, 2022

eileenmcnaughton Jan 7, 2022

eileenmcnaughton Jan 7, 2022

colemanw Jan 8, 2022

eileenmcnaughton commented Jan 8, 2022

colemanw commented Jan 8, 2022

eileenmcnaughton commented Jan 9, 2022

eileenmcnaughton Jan 9, 2022

colemanw Jan 9, 2022 •

edited

Loading

eileenmcnaughton Jan 9, 2022

colemanw Jan 10, 2022

colemanw commented Jan 10, 2022

eileenmcnaughton commented Jan 10, 2022

eileenmcnaughton commented Jan 13, 2022

		@@ -74,9 +74,6 @@ public static function create(&$params) {

		$dao = self::writeRecord($params);

		if (!empty($params['custom']) && is_array($params['custom'])) {

[REF] Improve APIv4 save functions #22403

[REF] Improve APIv4 save functions #22403

Conversation

colemanw commented Jan 7, 2022 • edited Loading

Overview

Before

After

civibot bot commented Jan 7, 2022

eileenmcnaughton Jan 7, 2022

Choose a reason for hiding this comment

eileenmcnaughton Jan 7, 2022

Choose a reason for hiding this comment

eileenmcnaughton Jan 7, 2022

Choose a reason for hiding this comment

colemanw Jan 8, 2022

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 8, 2022

colemanw commented Jan 8, 2022

eileenmcnaughton commented Jan 9, 2022

eileenmcnaughton Jan 9, 2022

Choose a reason for hiding this comment

colemanw Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Jan 9, 2022

Choose a reason for hiding this comment

colemanw Jan 10, 2022

Choose a reason for hiding this comment

colemanw commented Jan 10, 2022

eileenmcnaughton commented Jan 10, 2022

eileenmcnaughton commented Jan 13, 2022

colemanw commented Jan 7, 2022 •

edited

Loading

colemanw Jan 9, 2022 •

edited

Loading