From 121ec912ba6c0e30cbab311024396e09a87211e3 Mon Sep 17 00:00:00 2001
From: Coleman Watts <coleman@civicrm.org>
Date: Fri, 24 Jan 2020 09:16:08 -0500
Subject: [PATCH] Api4 - Use explicit adder functions rather than magicMethod

Api Actions had been relying on a magic method to provide getter/setter functions for params.
That works fine for simple get/set operations but it was also attempting to provide adder functions.
Those are more nuanced and the magic method didn't do a good job of understanding whether the param
was an indexed or unindexed array, and didn't do strict type checking.
Making our own adder functions gives better documentation, stricter checking of inputs and also
the convenience of variadic functions for adding several values at once.
---
 Civi/API/Kernel.php                        |  6 +++---
 Civi/Api4/Action/Setting/Get.php           | 13 +++++++++--
 Civi/Api4/Action/Setting/Revert.php        | 13 +++++++++--
 Civi/Api4/Action/Setting/Set.php           | 12 ++++++++++-
 Civi/Api4/Generic/AbstractAction.php       | 22 +++----------------
 Civi/Api4/Generic/AbstractCreateAction.php | 19 +++++++++++-----
 Civi/Api4/Generic/AbstractGetAction.php    | 13 +++++++++--
 Civi/Api4/Generic/AbstractQueryAction.php  | 17 ++++++++-------
 Civi/Api4/Generic/AbstractSaveAction.php   | 25 +++++++++++++++++++---
 Civi/Api4/Generic/AbstractUpdateAction.php | 18 ++++++++++++----
 Civi/Api4/Generic/BasicGetFieldsAction.php | 12 ++++++++++-
 Civi/Api4/Generic/BasicReplaceAction.php   | 23 ++++++++++++++++++--
 Civi/Api4/Generic/DAOEntity.php            |  2 +-
 13 files changed, 142 insertions(+), 53 deletions(-)

diff --git a/Civi/API/Kernel.php b/Civi/API/Kernel.php
index 651b0861a4..bb0f962be9 100644
--- a/Civi/API/Kernel.php
+++ b/Civi/API/Kernel.php
@@ -132,12 +132,12 @@ class Kernel {
   }
 
   /**
-   * Execute an API request.
+   * Execute an API v3 or v4 request.
    *
    * The request must be in canonical format. Exceptions will be propagated out.
    *
-   * @param array $apiRequest
-   * @return array
+   * @param array|\Civi\Api4\Generic\AbstractAction $apiRequest
+   * @return array|\Civi\Api4\Generic\Result
    * @throws \API_Exception
    * @throws \Civi\API\Exception\NotImplementedException
    * @throws \Civi\API\Exception\UnauthorizedException
diff --git a/Civi/Api4/Action/Setting/Get.php b/Civi/Api4/Action/Setting/Get.php
index f7ba1ee5d1..fd4a0ce3f0 100644
--- a/Civi/Api4/Action/Setting/Get.php
+++ b/Civi/Api4/Action/Setting/Get.php
@@ -26,8 +26,7 @@ use Civi\Api4\Generic\Result;
  * Get the value of one or more CiviCRM settings.
  *
  * @method array getSelect
- * @method $this addSelect(string $name)
- * @method $this setSelect(array $select)
+ * @method $this setSelect(array $settingNames)
  */
 class Get extends AbstractSettingAction {
 
@@ -71,4 +70,14 @@ class Get extends AbstractSettingAction {
     }
   }
 
+  /**
+   * Add one or more settings to be selected
+   * @param string ...$settingNames
+   * @return $this
+   */
+  public function addSelect(string ...$settingNames) {
+    $this->select = array_merge($this->select, $settingNames);
+    return $this;
+  }
+
 }
diff --git a/Civi/Api4/Action/Setting/Revert.php b/Civi/Api4/Action/Setting/Revert.php
index 811a854bad..11abc64db4 100644
--- a/Civi/Api4/Action/Setting/Revert.php
+++ b/Civi/Api4/Action/Setting/Revert.php
@@ -26,8 +26,7 @@ use Civi\Api4\Generic\Result;
  * Revert one or more CiviCRM settings to their default value.
  *
  * @method array getSelect
- * @method $this addSelect(string $name)
- * @method $this setSelect(array $select)
+ * @method $this setSelect(array $settingNames) Set settings to be reverted
  */
 class Revert extends AbstractSettingAction {
 
@@ -62,4 +61,14 @@ class Revert extends AbstractSettingAction {
     }
   }
 
+  /**
+   * Add one or more settings to be reverted
+   * @param string ...$settingNames
+   * @return $this
+   */
+  public function addSelect(string ...$settingNames) {
+    $this->select = array_merge($this->select, $settingNames);
+    return $this;
+  }
+
 }
diff --git a/Civi/Api4/Action/Setting/Set.php b/Civi/Api4/Action/Setting/Set.php
index b4d3727a0e..9353436c82 100644
--- a/Civi/Api4/Action/Setting/Set.php
+++ b/Civi/Api4/Action/Setting/Set.php
@@ -27,7 +27,6 @@ use Civi\Api4\Generic\Result;
  *
  * @method array getValues
  * @method $this setValues(array $value)
- * @method $this addValue(string $name, mixed $value)
  */
 class Set extends AbstractSettingAction {
 
@@ -60,4 +59,15 @@ class Set extends AbstractSettingAction {
     }
   }
 
+  /**
+   * Add an item to the values array
+   * @param string $settingName
+   * @param mixed $value
+   * @return $this
+   */
+  public function addValue($settingName, $value) {
+    $this->values[$settingName] = $value;
+    return $this;
+  }
+
 }
diff --git a/Civi/Api4/Generic/AbstractAction.php b/Civi/Api4/Generic/AbstractAction.php
index 2457009948..4168537718 100644
--- a/Civi/Api4/Generic/AbstractAction.php
+++ b/Civi/Api4/Generic/AbstractAction.php
@@ -151,7 +151,7 @@ abstract class AbstractAction implements \ArrayAccess {
    * @throws \API_Exception
    */
   public function setVersion($val) {
-    if ($val != 4) {
+    if ($val !== 4 && $val !== '4') {
       throw new \API_Exception('Cannot modify api version');
     }
     return $this;
@@ -172,7 +172,7 @@ abstract class AbstractAction implements \ArrayAccess {
   }
 
   /**
-   * Magic function to provide addFoo, getFoo and setFoo for params.
+   * Magic function to provide automatic getter/setter for params.
    *
    * @param $name
    * @param $arguments
@@ -185,10 +185,6 @@ abstract class AbstractAction implements \ArrayAccess {
       throw new \API_Exception('Unknown api parameter: ' . $name);
     }
     $mode = substr($name, 0, 3);
-    // Handle plural when adding to e.g. $values with "addValue" method.
-    if ($mode == 'add' && $this->paramExists($param . 's')) {
-      $param .= 's';
-    }
     if ($this->paramExists($param)) {
       switch ($mode) {
         case 'get':
@@ -197,18 +193,6 @@ abstract class AbstractAction implements \ArrayAccess {
         case 'set':
           $this->$param = $arguments[0];
           return $this;
-
-        case 'add':
-          if (!is_array($this->$param)) {
-            throw new \API_Exception('Cannot add to non-array param');
-          }
-          if (array_key_exists(1, $arguments)) {
-            $this->{$param}[$arguments[0]] = $arguments[1];
-          }
-          else {
-            $this->{$param}[] = $arguments[0];
-          }
-          return $this;
       }
     }
     throw new \API_Exception('Unknown api parameter: ' . $name);
@@ -221,12 +205,12 @@ abstract class AbstractAction implements \ArrayAccess {
    * This is basically the outer wrapper for api v4.
    *
    * @return \Civi\Api4\Generic\Result
+   * @throws \API_Exception
    * @throws \Civi\API\Exception\UnauthorizedException
    */
   public function execute() {
     /** @var \Civi\API\Kernel $kernel */
     $kernel = \Civi::service('civi_api_kernel');
-
     $result = $kernel->runRequest($this);
     if ($this->debug && (!$this->checkPermissions || \CRM_Core_Permission::check('view debug output'))) {
       $result->debug = array_merge($result->debug, $this->_debugOutput);
diff --git a/Civi/Api4/Generic/AbstractCreateAction.php b/Civi/Api4/Generic/AbstractCreateAction.php
index 20551e5b09..416e15f056 100644
--- a/Civi/Api4/Generic/AbstractCreateAction.php
+++ b/Civi/Api4/Generic/AbstractCreateAction.php
@@ -25,7 +25,6 @@ namespace Civi\Api4\Generic;
  * Base class for all "Create" api actions.
  *
  * @method $this setValues(array $values) Set all field values from an array of key => value pairs.
- * @method $this addValue($field, $value) Set field value.
  * @method array getValues() Get field values.
  *
  * @package Civi\Api4\Generic
@@ -40,12 +39,22 @@ abstract class AbstractCreateAction extends AbstractAction {
   protected $values = [];
 
   /**
-   * @param string $key
-   *
+   * @param string $fieldName
    * @return mixed|null
    */
-  public function getValue($key) {
-    return isset($this->values[$key]) ? $this->values[$key] : NULL;
+  public function getValue(string $fieldName) {
+    return isset($this->values[$fieldName]) ? $this->values[$fieldName] : NULL;
+  }
+
+  /**
+   * Add a field value.
+   * @param string $fieldName
+   * @param mixed $value
+   * @return $this
+   */
+  public function addValue(string $fieldName, $value) {
+    $this->values[$fieldName] = $value;
+    return $this;
   }
 
   /**
diff --git a/Civi/Api4/Generic/AbstractGetAction.php b/Civi/Api4/Generic/AbstractGetAction.php
index adf84fb43b..1e91ec928b 100644
--- a/Civi/Api4/Generic/AbstractGetAction.php
+++ b/Civi/Api4/Generic/AbstractGetAction.php
@@ -28,8 +28,7 @@ use Civi\Api4\Utils\SelectUtil;
  *
  * @package Civi\Api4\Generic
  *
- * @method $this addSelect(string $select)
- * @method $this setSelect(array $selects)
+ * @method $this setSelect(array $selects) Set array of fields to be selected (wildcard * allowed)
  * @method array getSelect()
  */
 abstract class AbstractGetAction extends AbstractQueryAction {
@@ -154,4 +153,14 @@ abstract class AbstractGetAction extends AbstractQueryAction {
     return FALSE;
   }
 
+  /**
+   * Add one or more fields to be selected (wildcard * allowed)
+   * @param string ...$fieldNames
+   * @return $this
+   */
+  public function addSelect(string ...$fieldNames) {
+    $this->select = array_merge($this->select, $fieldNames);
+    return $this;
+  }
+
 }
diff --git a/Civi/Api4/Generic/AbstractQueryAction.php b/Civi/Api4/Generic/AbstractQueryAction.php
index 4088545bd1..96d442ecc8 100644
--- a/Civi/Api4/Generic/AbstractQueryAction.php
+++ b/Civi/Api4/Generic/AbstractQueryAction.php
@@ -79,17 +79,17 @@ abstract class AbstractQueryAction extends AbstractAction {
   protected $offset = 0;
 
   /**
-   * @param string $field
+   * @param string $fieldName
    * @param string $op
    * @param mixed $value
    * @return $this
    * @throws \API_Exception
    */
-  public function addWhere($field, $op, $value = NULL) {
+  public function addWhere(string $fieldName, string $op, $value = NULL) {
     if (!in_array($op, \CRM_Core_DAO::acceptedSQLOperators())) {
       throw new \API_Exception('Unsupported operator');
     }
-    $this->where[] = [$field, $op, $value];
+    $this->where[] = [$fieldName, $op, $value];
     return $this;
   }
 
@@ -103,7 +103,7 @@ abstract class AbstractQueryAction extends AbstractAction {
    * @return $this
    * @throws \API_Exception
    */
-  public function addClause($operator, $condition1) {
+  public function addClause(string $operator, $condition1) {
     if (!is_array($condition1[0])) {
       $condition1 = array_slice(func_get_args(), 1);
     }
@@ -112,17 +112,18 @@ abstract class AbstractQueryAction extends AbstractAction {
   }
 
   /**
-   * @param string $field
+   * Adds to the orderBy clause
+   * @param string $fieldName
    * @param string $direction
    * @return $this
    */
-  public function addOrderBy($field, $direction = 'ASC') {
-    $this->orderBy[$field] = $direction;
+  public function addOrderBy(string $fieldName, $direction = 'ASC') {
+    $this->orderBy[$fieldName] = $direction;
     return $this;
   }
 
   /**
-   * A human-readable where clause, for the reading enjoyment of you humans.
+   * Produces a human-readable where clause, for the reading enjoyment of you humans.
    *
    * @param array $whereClause
    * @param string $op
diff --git a/Civi/Api4/Generic/AbstractSaveAction.php b/Civi/Api4/Generic/AbstractSaveAction.php
index 170d63bd59..a3cc132767 100644
--- a/Civi/Api4/Generic/AbstractSaveAction.php
+++ b/Civi/Api4/Generic/AbstractSaveAction.php
@@ -24,11 +24,9 @@ namespace Civi\Api4\Generic;
 /**
  * Base class for all "Save" api actions.
  *
- * @method $this setRecords(array $records) Array of records.
- * @method $this addRecord($record) Add a record to update.
+ * @method $this setRecords(array $records) Set array of records to be saved.
  * @method array getRecords()
  * @method $this setDefaults(array $defaults) Array of defaults.
- * @method $this addDefault($name, $value) Add a default value.
  * @method array getDefaults()
  * @method $this setReload(bool $reload) Specify whether complete objects will be returned after saving.
  * @method bool getReload()
@@ -106,4 +104,25 @@ abstract class AbstractSaveAction extends AbstractAction {
     return $this->idField;
   }
 
+  /**
+   * Add one or more records to be saved.
+   * @param array ...$records
+   * @return $this
+   */
+  public function addRecord(array ...$records) {
+    $this->records = array_merge($this->records, $records);
+    return $this;
+  }
+
+  /**
+   * Set default value for a field.
+   * @param string $fieldName
+   * @param mixed $defaultValue
+   * @return $this
+   */
+  public function addDefault(string $fieldName, $defaultValue) {
+    $this->defaults[$fieldName] = $defaultValue;
+    return $this;
+  }
+
 }
diff --git a/Civi/Api4/Generic/AbstractUpdateAction.php b/Civi/Api4/Generic/AbstractUpdateAction.php
index d0f81572de..b3fe4d8635 100644
--- a/Civi/Api4/Generic/AbstractUpdateAction.php
+++ b/Civi/Api4/Generic/AbstractUpdateAction.php
@@ -25,7 +25,6 @@ namespace Civi\Api4\Generic;
  * Base class for all "Update" api actions
  *
  * @method $this setValues(array $values) Set all field values from an array of key => value pairs.
- * @method $this addValue($field, $value) Set field value.
  * @method array getValues() Get field values.
  * @method $this setReload(bool $reload) Specify whether complete objects will be returned after saving.
  * @method bool getReload()
@@ -53,12 +52,23 @@ abstract class AbstractUpdateAction extends AbstractBatchAction {
   protected $reload = FALSE;
 
   /**
-   * @param string $key
+   * @param string $fieldName
    *
    * @return mixed|null
    */
-  public function getValue($key) {
-    return isset($this->values[$key]) ? $this->values[$key] : NULL;
+  public function getValue(string $fieldName) {
+    return isset($this->values[$fieldName]) ? $this->values[$fieldName] : NULL;
+  }
+
+  /**
+   * Add an item to the values array
+   * @param string $fieldName
+   * @param mixed $value
+   * @return $this
+   */
+  public function addValue(string $fieldName, $value) {
+    $this->values[$fieldName] = $value;
+    return $this;
   }
 
 }
diff --git a/Civi/Api4/Generic/BasicGetFieldsAction.php b/Civi/Api4/Generic/BasicGetFieldsAction.php
index 5c2b0ccb11..6bf1078816 100644
--- a/Civi/Api4/Generic/BasicGetFieldsAction.php
+++ b/Civi/Api4/Generic/BasicGetFieldsAction.php
@@ -30,7 +30,6 @@ use Civi\Api4\Utils\ActionUtil;
  * @method $this setLoadOptions(bool $value)
  * @method bool getLoadOptions()
  * @method $this setAction(string $value)
- * @method $this addValue(string $value)
  * @method $this setValues(array $values)
  * @method array getValues()
  */
@@ -127,6 +126,17 @@ class BasicGetFieldsAction extends BasicGetAction {
     return $sub[$this->action] ?? $this->action;
   }
 
+  /**
+   * Add an item to the values array
+   * @param string $fieldName
+   * @param mixed $value
+   * @return $this
+   */
+  public function addValue(string $fieldName, $value) {
+    $this->values[$fieldName] = $value;
+    return $this;
+  }
+
   public function fields() {
     return [
       [
diff --git a/Civi/Api4/Generic/BasicReplaceAction.php b/Civi/Api4/Generic/BasicReplaceAction.php
index 1c71536279..31f12a44ac 100644
--- a/Civi/Api4/Generic/BasicReplaceAction.php
+++ b/Civi/Api4/Generic/BasicReplaceAction.php
@@ -28,10 +28,8 @@ use Civi\Api4\Utils\ActionUtil;
  * Given a set of records, will appropriately update the database.
  *
  * @method $this setRecords(array $records) Array of records.
- * @method $this addRecord($record) Add a record to update.
  * @method array getRecords()
  * @method $this setDefaults(array $defaults) Array of defaults.
- * @method $this addDefault($name, $value) Add a default value.
  * @method array getDefaults()
  * @method $this setReload(bool $reload) Specify whether complete objects will be returned after saving.
  * @method bool getReload()
@@ -130,4 +128,25 @@ class BasicReplaceAction extends AbstractBatchAction {
     }
   }
 
+  /**
+   * Set default value for a field.
+   * @param string $fieldName
+   * @param mixed $defaultValue
+   * @return $this
+   */
+  public function addDefault(string $fieldName, $defaultValue) {
+    $this->defaults[$fieldName] = $defaultValue;
+    return $this;
+  }
+
+  /**
+   * Add one or more records
+   * @param array ...$records
+   * @return $this
+   */
+  public function addRecord(array ...$records) {
+    $this->records = array_merge($this->records, $records);
+    return $this;
+  }
+
 }
diff --git a/Civi/Api4/Generic/DAOEntity.php b/Civi/Api4/Generic/DAOEntity.php
index 9fc8bb161b..38888227c6 100644
--- a/Civi/Api4/Generic/DAOEntity.php
+++ b/Civi/Api4/Generic/DAOEntity.php
@@ -34,7 +34,7 @@ abstract class DAOEntity extends AbstractEntity {
   }
 
   /**
-   * @return DAOGetAction
+   * @return DAOSaveAction
    */
   public static function save() {
     return new DAOSaveAction(static::class, __FUNCTION__);
-- 
2.25.1