Merge pull request #17906 from seamuslee001/lab_core_1846

#1846 Make DAO upgrade safe

CRM/Core/DAO.php

@@ -530,6 +530,43 @@ class CRM_Core_DAO extends DB_DataObject {
     return $result;
   }
 
+  /**
+   * Returns all usable fields, indexed by name.
+   *
+   * This function differs from fields() in that it indexes by name rather than unique_name.
+   *
+   * It excludes fields not added yet by pending upgrades.
+   * This avoids problems with trying to SELECT a field that exists in code but has not yet been added to the db.
+   *
+   * @param bool $checkPermissions
+   *   Filter by field permissions.
+   * @return array
+   */
+  public static function getSupportedFields($checkPermissions = FALSE) {
+    $fields = array_column((array) static::fields(), NULL, 'name');
+
+    // Exclude fields yet not added by pending upgrades
+    $dbVer = \CRM_Core_BAO_Domain::version();
+    if ($fields && version_compare($dbVer, \CRM_Utils_System::version()) < 0) {
+      $fields = array_filter($fields, function($field) use ($dbVer) {
+        $add = $field['add'] ?? '1.0.0';
+        if (substr_count($add, '.') < 2) {
+          $add .= '.alpha1';
+        }
+        return version_compare($dbVer, $add, '>=');
+      });
+    }
+
+    // Exclude fields the user does not have permission for
+    if ($checkPermissions) {
+      $fields = array_filter($fields, function($field) {
+        return empty($field['permission']) || CRM_Core_Permission::check($field['permission']);
+      });
+    }
+
+    return $fields;
+  }
+
   /**
    * Get/set an associative array of table columns
    *

Civi/Api4/Service/Spec/SpecGatherer.php

@@ -166,7 +166,7 @@ class SpecGatherer {
   private function getDAOFields($entityName) {
     $bao = CoreUtil::getBAOFromApiName($entityName);
 
-    return $bao::fields();
+    return $bao::getSupportedFields();
   }
 
 }

api/v3/Setting.php

@@ -195,7 +195,7 @@ function _civicrm_api3_setting_revert_spec(&$params) {
  * Revert settings to defaults.
  *
  * @param array $params
- *
+ * @deprecated
  * @return array
  * @throws \CiviCRM_API3_Exception
  * @throws \Exception
@@ -240,6 +240,15 @@ function _civicrm_api3_setting_fill_spec(&$params) {
   ];
 }
 
+/**
+ * Declare deprecated api functions.
+ *
+ * @return array
+ */
+function _civicrm_api3_setting_deprecation() {
+  return ['fill' => 'Setting "fill" is no longer necessary.'];
+}
+
 /**
  * Create or update a setting.
  *

api/v3/utils.php

@@ -294,7 +294,7 @@ function _civicrm_api3_load_DAO($entity) {
  *   return the DAO name to manipulate this function
  *   eg. "civicrm_api3_contact_create" or "Contact" will return "CRM_Contact_BAO_Contact"
  *
- * @return mixed|string
+ * @return CRM_Core_DAO|string
  */
 function _civicrm_api3_get_DAO($name) {
   if (strpos($name, 'civicrm_api3') !== FALSE) {
@@ -1878,15 +1878,19 @@ function _civicrm_api_get_fields($entity, $unique = FALSE, &$params = []) {
   if (empty($dao)) {
     return [];
   }
-  $d = new $dao();
-  $fields = $d->fields();
+  $fields = $dao::fields();
+  $supportedFields = $dao::getSupportedFields();
 
-  foreach ($fields as $name => &$field) {
+  foreach ($fields as $name => $field) {
     // Denote as core field
-    $field['is_core_field'] = TRUE;
+    $fields[$name]['is_core_field'] = TRUE;
     // Set html attributes for text fields
     if (isset($field['html'])) {
-      $field['html'] += (array) $d::makeAttribute($field);
+      $fields[$name]['html'] += (array) $dao::makeAttribute($field);
+    }
+    // Delete field if not supported by current db schema (prevents errors when there are pending db updates)
+    if (!isset($supportedFields[$field['name']])) {
+      unset($fields[$name]);
     }
   }
 

sql/test_data_second_domain.mysql

@@ -963,4 +963,4 @@ INSERT INTO civicrm_navigation
 VALUES
     ( @domainID, CONCAT('civicrm/report/instance/', @instanceID,'&reset=1'), 'Mailing Detail Report', 'Mailing Detail Report', 'administer CiviMail', 'OR', @reportlastID, '1', NULL, @instanceID+2 );
 UPDATE civicrm_report_instance SET navigation_id = LAST_INSERT_ID() WHERE id = @instanceID;
-UPDATE civicrm_domain SET version = '5.27.alpha1';
+UPDATE civicrm_domain SET version = '5.28.alpha1';

tests/phpunit/CRM/Core/DAOTest.php

@@ -516,4 +516,30 @@ class CRM_Core_DAOTest extends CiviUnitTestCase {
     $this->fail('String not altered');
   }
 
+  public function testSupportedFields() {
+    // Hack a different db version which will trigger getSupportedFields to filter out newer fields
+    \CRM_Core_DAO::$_dbColumnValueCache['CRM_Core_DAO_Domain']['id'][1]['version'] = '5.26.0';
+
+    $customGroupFields = CRM_Core_DAO_CustomGroup::getSupportedFields();
+    // 'icon' was added in 5.28
+    $this->assertArrayNotHasKey('icon', $customGroupFields);
+
+    // Remove domain version override:
+    \CRM_Core_DAO::$_dbColumnValueCache = NULL;
+
+    $activityFields = CRM_Activity_DAO_Activity::getSupportedFields();
+    // Fields should be indexed by name not unique_name (which is "activity_id")
+    $this->assertEquals('id', $activityFields['id']['name']);
+
+    $customGroupFields = CRM_Core_DAO_CustomGroup::getSupportedFields();
+    $this->assertArrayHasKey('icon', $customGroupFields);
+
+    \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'view all contacts'];
+    $contactFields = CRM_Contact_DAO_Contact::getSupportedFields();
+    $this->assertArrayHasKey('api_key', $contactFields);
+
+    $permissionedContactFields = CRM_Contact_DAO_Contact::getSupportedFields(TRUE);
+    $this->assertArrayNotHasKey('api_key', $permissionedContactFields);
+  }
+
 }

tests/phpunit/api/v3/SettingTest.php

@@ -32,21 +32,15 @@ class api_v3_SettingTest extends CiviUnitTestCase {
   public function setUp() {
     parent::setUp();
     $params = [
-      'name' => 'Default Domain Name',
-      'domain_version' => '4.7',
+      'name' => __CLASS__ . 'Second Domain',
+      'domain_version' => CRM_Utils_System::version(),
     ];
     $result = $this->callAPISuccess('domain', 'get', $params);
     if (empty($result['id'])) {
       $result = $this->callAPISuccess('domain', 'create', $params);
     }
-
-    $params['name'] = 'Second Domain';
-    $result = $this->callAPISuccess('domain', 'get', $params);
-    if (empty($result['id'])) {
-      $result = $this->callAPISuccess('domain', 'create', $params);
-    }
     $this->_domainID2 = $result['id'];
-    $params['name'] = 'A-team domain';
+    $params['name'] = __CLASS__ . 'Third domain';
     $result = $this->callAPISuccess('domain', 'get', $params);
     if (empty($result['id'])) {
       $result = $this->callAPISuccess('domain', 'create', $params);
@@ -60,7 +54,7 @@ class api_v3_SettingTest extends CiviUnitTestCase {
     CRM_Utils_Hook::singleton()->reset();
     parent::tearDown();
     $this->callAPISuccess('system', 'flush', []);
-    $this->quickCleanup(['civicrm_domain']);
+    CRM_Core_DAO::executeQuery('DELETE FROM civicrm_domain WHERE name LIKE "' . __CLASS__ . '%"');
   }
 
   /**
@@ -300,10 +294,10 @@ class api_v3_SettingTest extends CiviUnitTestCase {
     ];
     $result = $this->callAPIAndDocument('setting', 'create', $params, __FUNCTION__, __FILE__, $description, 'CreateAllDomains');
 
-    $this->assertEquals(1, $result['values'][2]['uniq_email_per_site']);
-    $this->assertEquals(1, $result['values'][1]['uniq_email_per_site']);
-    $this->assertArrayHasKey(3, $result['values'], 'Domain create probably failed Debug this IF domain test is passing');
-    $this->assertEquals(1, $result['values'][3]['uniq_email_per_site'], 'failed to set setting for domain 3.');
+    $this->assertEquals(1, $result['values'][$this->_domainID2]['uniq_email_per_site']);
+    $this->assertEquals(1, $result['values'][$this->_currentDomain]['uniq_email_per_site']);
+    $this->assertArrayHasKey($this->_domainID3, $result['values'], 'Domain create probably failed Debug this IF domain test is passing');
+    $this->assertEquals(1, $result['values'][$this->_domainID3]['uniq_email_per_site'], 'failed to set setting for domain 3.');
 
     $params = [
       'domain_id' => 'all',
@@ -313,27 +307,27 @@ class api_v3_SettingTest extends CiviUnitTestCase {
     $description = "Shows getting a variable for all domains.";
     $result = $this->callAPIAndDocument('setting', 'get', $params, __FUNCTION__, __FILE__, $description, 'GetAllDomains');
 
-    $this->assertEquals(1, $result['values'][2]['uniq_email_per_site']);
-    $this->assertEquals(1, $result['values'][1]['uniq_email_per_site']);
-    $this->assertEquals(1, $result['values'][3]['uniq_email_per_site']);
+    $this->assertEquals(1, $result['values'][$this->_domainID2]['uniq_email_per_site']);
+    $this->assertEquals(1, $result['values'][$this->_currentDomain]['uniq_email_per_site']);
+    $this->assertEquals(1, $result['values'][$this->_domainID3]['uniq_email_per_site']);
 
     $params = [
-      'domain_id' => [1, 3],
+      'domain_id' => [$this->_currentDomain, $this->_domainID3],
       'uniq_email_per_site' => 0,
     ];
     $description = "Shows setting a variable for specified domains.";
     $result = $this->callAPIAndDocument('setting', 'create', $params, __FUNCTION__, __FILE__, $description, 'CreateSpecifiedDomains');
 
-    $this->assertEquals(0, $result['values'][3]['uniq_email_per_site']);
-    $this->assertEquals(0, $result['values'][1]['uniq_email_per_site']);
+    $this->assertEquals(0, $result['values'][$this->_domainID3]['uniq_email_per_site']);
+    $this->assertEquals(0, $result['values'][$this->_currentDomain]['uniq_email_per_site']);
     $params = [
-      'domain_id' => [1, 2],
+      'domain_id' => [$this->_currentDomain, $this->_domainID2],
       'return' => ['uniq_email_per_site'],
     ];
     $description = "Shows getting a variable for specified domains.";
     $result = $this->callAPIAndDocument('setting', 'get', $params, __FUNCTION__, __FILE__, $description, 'GetSpecifiedDomains');
-    $this->assertEquals(1, $result['values'][2]['uniq_email_per_site']);
-    $this->assertEquals(0, $result['values'][1]['uniq_email_per_site']);
+    $this->assertEquals(1, $result['values'][$this->_domainID2]['uniq_email_per_site']);
+    $this->assertEquals(0, $result['values'][$this->_currentDomain]['uniq_email_per_site']);
 
   }
 
@@ -524,8 +518,8 @@ class api_v3_SettingTest extends CiviUnitTestCase {
    */
   public function testDefaults() {
     $domparams = [
-      'name' => 'B Team Domain',
-      'domain_version' => '4.7',
+      'name' => __CLASS__ . 'B Team Domain',
+      'domain_version' => CRM_Utils_System::version(),
     ];
     $dom = $this->callAPISuccess('domain', 'create', $domparams);
     $params = [