API - Consistently save custom data for v3 & v4 #24036

colemanw · 2022-07-21T15:07:16Z

Overview

Before

Mailing custom fields saved for v3 but not v4.

After

Works for both, plus more cleanup.

Technical Details

APIv3 has a fallback for saving custom data in the API layer if the BAO doesn't handle it. APIv4 does not, and relies on the BAO to always work. So we really need to get our BAOs consistently saving custom data. The best way to do that is for them to use writeRecord instead of ad-hoc create and add functions. I've been making an effort to mark create and add functions @deprecated which triggers the API to ignore them.

Comments

Ideally we should remove custom data handling from v3 but this PR doesn't go quite that far.

civibot · 2022-07-21T15:07:19Z

(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).

Fixes dev/core#3747 It's confusing to handle saving custom data in the APIv3 layer, because APIv4 expects the BAO to handle it. Until every BAO consistently uses WriteRecord this will continue to be a pain point.

jensschuppe · 2022-07-22T07:58:18Z

I can confirm this fixes the issue for Mailing entities - didn't test any other scenario than described in the issue though.

eileenmcnaughton · 2022-07-23T17:19:03Z

CRM/ACL/BAO/ACLEntityRole.php

@@ -35,6 +35,7 @@ public static function entityTables(): array {
  /**
   * @param array $params
   *
+   * @deprecated
   * @return CRM_ACL_BAO_ACLEntityRole
   */
  public static function create(&$params) {


OK - already calls writeRecord

eileenmcnaughton · 2022-07-23T17:19:41Z

CRM/Core/BAO/ActionSchedule.php

@@ -174,6 +174,7 @@ public static function getList($filterMapping = NULL, $filterValue = NULL): arra
   * @param array $params
   *   An assoc array of name/value pairs.
   *
+   * @deprecated


eileenmcnaughton · 2022-07-23T17:20:07Z

CRM/Core/BAO/Domain.php

@@ -149,6 +149,7 @@ public function getLocationValues() {
   * @param array $params
   * @param int $id
   *
+   * @deprecated


eileenmcnaughton · 2022-07-23T17:21:03Z

CRM/Core/BAO/Domain.php

@@ -160,6 +161,7 @@ public static function edit($params, $id): CRM_Core_DAO_Domain {
  /**
   * Create or update domain.
   *
+   * @deprecated


OK - not called from api anyway

eileenmcnaughton · 2022-07-23T17:21:29Z

CRM/Core/BAO/RecurringEntity.php

@@ -144,6 +144,7 @@ public static function setStatus($status) {
  /**
   * Create or update a RecurringEntity.
   *
+   * @deprecated


eileenmcnaughton · 2022-07-23T17:22:01Z

CRM/Core/BAO/Website.php

@@ -26,6 +26,7 @@ class CRM_Core_BAO_Website extends CRM_Core_DAO_Website {
   *
   * @param array $params
   *
+   * @deprecated


eileenmcnaughton · 2022-07-23T17:22:22Z

CRM/Core/BAO/Website.php

@@ -46,7 +45,7 @@ public static function create($params) {
   */
  public static function add($params) {
    CRM_Core_Error::deprecatedFunctionWarning('use apiv4');
-    return self::create($params);
+    return self::writeRecord($params);


eileenmcnaughton · 2022-07-23T17:23:09Z

I started eyeballing this & commented where I've checked.

eileenmcnaughton · 2022-07-23T17:27:54Z

CRM/Mailing/BAO/Mailing.php

   *
   * @return CRM_Mailing_DAO_Mailing
   * @throws \Civi\API\Exception\UnauthorizedException
   */
-  public static function add(&$params, $ids = []) {
-    $id = $params['id'] ?? $ids['mailing_id'] ?? NULL;
+  public static function add($params) {


ok - ids was deprecated noisily in year zero (2020)

eileenmcnaughton · 2022-07-23T17:29:13Z

CRM/Mailing/BAO/Mailing.php

      isset($params['from_email'])
    ) {
      $params['replyto_email'] = $params['from_email'];
    }
-    $mailing->copyValues($params);


OK - changes to this function make sense

eileenmcnaughton · 2022-07-23T17:29:42Z

CRM/Member/BAO/MembershipBlock.php

@@ -19,6 +19,7 @@ class CRM_Member_BAO_MembershipBlock extends CRM_Member_DAO_MembershipBlock {
  /**
   * Create or update a MembershipBlock.
   *
+   * @deprecated


eileenmcnaughton · 2022-07-23T17:30:40Z

CRM/Pledge/BAO/PledgeBlock.php

-   *
-   * @return CRM_Pledge_BAO_PledgeBlock
+   * @deprecated
+   * @return CRM_Pledge_DAO_PledgeBlock
   */


OK - this is long but completely lacking in goodness

eileenmcnaughton · 2022-07-23T17:31:28Z

CRM/Pledge/BAO/PledgeBlock.php

-   *
-   * @return object
+   * @deprecated
+   * @return CRM_Pledge_DAO_PledgeBlock
   */
  public static function add($params) {


OK - this has some non-api-worthy horror show but nothing the api needs

eileenmcnaughton · 2022-07-23T17:36:37Z

@colemanw can you explain this bit - https://github.com/civicrm/civicrm-core/pull/24036/files#diff-66c5190e94fdb11fb2c74061a039d80cfe7908cff12cce3a4244021afd57f569R1337-R1356

My read is that anything that calls writeRecord should be in that list - so most of the ones you touched, like MembershipBlock should be in there - but it isn't?

eileenmcnaughton · 2022-07-23T17:37:44Z

Also - we could probably add a deprecation notice into that apiv3 custom field handling (in a follow up PR)

colemanw · 2022-07-25T23:44:14Z

@eileenmcnaughton everything that's annotated @deprecated doesn't need to be in the list. The list is just for oddballs that do handle custom data but don't have the annotation.

eileenmcnaughton · 2022-07-26T14:04:02Z

OK - well tests pass so ...

civibot bot added the master label Jul 21, 2022

APIv3 - Don't handle custom data already handled by BAO

f54abaa

Fixes dev/core#3747 It's confusing to handle saving custom data in the APIv3 layer, because APIv4 expects the BAO to handle it. Until every BAO consistently uses WriteRecord this will continue to be a pain point.

colemanw force-pushed the v3Custom branch from 4a83098 to f54abaa Compare July 21, 2022 20:11

eileenmcnaughton reviewed Jul 23, 2022

View reviewed changes

eileenmcnaughton merged commit 1c41a31 into civicrm:master Jul 26, 2022

eileenmcnaughton deleted the v3Custom branch July 26, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API - Consistently save custom data for v3 & v4 #24036

API - Consistently save custom data for v3 & v4 #24036

colemanw commented Jul 21, 2022 •

edited

Loading

civibot bot commented Jul 21, 2022

jensschuppe commented Jul 22, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton commented Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton Jul 23, 2022

eileenmcnaughton commented Jul 23, 2022

eileenmcnaughton commented Jul 23, 2022

colemanw commented Jul 25, 2022

eileenmcnaughton commented Jul 26, 2022

API - Consistently save custom data for v3 & v4 #24036

API - Consistently save custom data for v3 & v4 #24036

Conversation

colemanw commented Jul 21, 2022 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 21, 2022

jensschuppe commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jul 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jul 23, 2022

eileenmcnaughton commented Jul 23, 2022

colemanw commented Jul 25, 2022

eileenmcnaughton commented Jul 26, 2022

colemanw commented Jul 21, 2022 •

edited

Loading