From bdcb5b1f60ce341c4b9a0d209a43e73d10f8e351 Mon Sep 17 00:00:00 2001
From: colemanw <coleman@civicrm.org>
Date: Thu, 20 Jul 2023 11:10:42 -0400
Subject: [PATCH] CRM_Admin_Form - general code cleanup

---
 CRM/Admin/Form.php | 52 ++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/CRM/Admin/Form.php b/CRM/Admin/Form.php
index 70ab548a51..99b82b333d 100644
--- a/CRM/Admin/Form.php
+++ b/CRM/Admin/Form.php
@@ -37,7 +37,7 @@ class CRM_Admin_Form extends CRM_Core_Form {
   /**
    * The name of the BAO object for this form.
    *
-   * @var string
+   * @var CRM_Core_DAO|string
    */
   protected $_BAOName;
 
@@ -51,8 +51,8 @@ class CRM_Admin_Form extends CRM_Core_Form {
   /**
    * Note: This type of form was traditionally embedded in a page, with values like _id and _action
    * being `set()` by the page controller.
-   * Nowadays the preferred approach is to place these forms at their own url, so this function
-   * handles both scenarios. It will retrieve id either from a value stored by the page controller
+   * Nowadays the preferred approach is to place these forms at their own url.
+   * This function can handle either scenario. It will retrieve `id` either from a value stored by the page controller
    * if embedded, or from the url if standalone.
    */
   public function preProcess() {
@@ -62,34 +62,25 @@ class CRM_Admin_Form extends CRM_Core_Form {
     // Lookup id from URL or stored value in controller
     $this->_id = CRM_Utils_Request::retrieve('id', 'Positive', $this);
 
+    // If embedded in a page, this will have been assigned
     $this->_BAOName = $this->get('BAOName');
-    // If BAOName not explicitly set, look it up from the api entity name
+    // Otherwise, look it up from the api entity name
     if (!$this->_BAOName) {
       $this->_BAOName = CRM_Core_DAO_AllCoreTables::getBAOClassName(CRM_Core_DAO_AllCoreTables::getFullName($this->getDefaultEntity()));
     }
-    $this->_values = [];
-    if (isset($this->_id)) {
-      $params = ['id' => $this->_id];
-      // this is needed if the form is outside the CRM name space
-      $baoName = $this->_BAOName;
-      $baoName::retrieve($params, $this->_values);
-    }
+    $this->retrieveValues();
   }
 
   /**
    * Set default values for the form. Note that in edit/view mode
    * the default values are retrieved from the database
    *
-   *
    * @return array
    */
   public function setDefaultValues() {
-    // Fetch defaults from the db
-    if (!empty($this->_id) && empty($this->_values) && CRM_Utils_Rule::positiveInteger($this->_id)) {
-      $this->_values = [];
-      $params = ['id' => $this->_id];
-      $baoName = $this->_BAOName;
-      $baoName::retrieve($params, $this->_values);
+    // Fetch defaults from the db if not already retrieved
+    if (empty($this->_values)) {
+      $this->retrieveValues();
     }
     $defaults = $this->_values;
 
@@ -102,14 +93,12 @@ class CRM_Admin_Form extends CRM_Core_Form {
       }
     }
 
-    if ($this->_action == CRM_Core_Action::DELETE &&
-      isset($defaults['name'])
-    ) {
+    if ($this->_action == CRM_Core_Action::DELETE && isset($defaults['name'])) {
       $this->assign('delName', $defaults['name']);
     }
 
-    // its ok if there is no element called is_active
-    $defaults['is_active'] = ($this->_id) ? CRM_Utils_Array::value('is_active', $defaults) : 1;
+    // Field is_active should default to TRUE (if there is no such field, this value will be ignored)
+    $defaults['is_active'] = ($this->_id) ? $defaults['is_active'] ?? 1 : 1;
     if (!empty($defaults['parent_id'])) {
       $this->assign('is_parent', TRUE);
     }
@@ -144,4 +133,21 @@ class CRM_Admin_Form extends CRM_Core_Form {
     }
   }
 
+  /**
+   * Retrieve entity from the database.
+   *
+   * TODO: Add flag to allow forms to opt-in to using API::get instead of BAO::retrieve
+   *
+   * @return array
+   */
+  protected function retrieveValues(): array {
+    $this->_values = [];
+    if (isset($this->_id) && CRM_Utils_Rule::positiveInteger($this->_id)) {
+      $params = ['id' => $this->_id];
+      // FIXME: `retrieve` function is deprecated :(
+      $this->_BAOName::retrieve($params, $this->_values);
+    }
+    return $this->_values;
+  }
+
 }
-- 
2.25.1