CRM-20199 Remove poor join on option_value table when doing contribut…

…ion search.

This involves setting payment instrument to exportable & remove hacks that were used in
place of doing that, and adding new logic to the order by to allow it to order without
joining the option value table (using the field() function).

I have observed one small regression which is the Qill showing 'Payment Method ID'
instead of payment method. I'm committed to fixing that but I would push
to merge this in advance of that because we are in the best place in the monthly
cycle to merge this and that is a very minor fallout.

CRM/Contact/BAO/Query.php

@@ -653,8 +653,7 @@ public function selectClause($apiEntity = NULL) {
       if (
         (substr($name, 0, 12) == 'participant_') ||
         (substr($name, 0, 7) == 'pledge_') ||
-        (substr($name, 0, 5) == 'case_') ||
-        (substr($name, 0, 8) == 'payment_')
+        (substr($name, 0, 5) == 'case_')
       ) {
         continue;
       }
@@ -6203,10 +6202,17 @@ protected function prepareOrderBy($sort, $sortByChar, $sortOrder, $additionalFro
     $order = trim(str_replace('ORDER BY', '', $order));
 
     // hack for order clause
-    $fieldOrder = explode(' ', $order);
-    $field = $fieldOrder[0];
+    if (!empty($orderByArray)) {
+      $order = implode(', ', $orderByArray);
+    }
+    else {
+      $orderByArray = explode(',', $order);
+    }
+    foreach ($orderByArray as $orderByClause) {
+      $orderByClauseParts = explode(' ', trim($orderByClause));
+      $field = $orderByClauseParts[0];
+      $direction = isset($orderByClauseParts[1]) ? $orderByClauseParts[1] : 'asc';
 
-    if ($field) {
       switch ($field) {
         case 'city':
         case 'postal_code':
@@ -6229,25 +6235,47 @@ protected function prepareOrderBy($sort, $sortByChar, $sortOrder, $additionalFro
           break;
 
         default:
-          //CRM-12565 add "`" around $field if it is a pseudo constant
-          foreach ($this->_pseudoConstantsSelect as $key => $value) {
-            if (!empty($value['element']) && $value['element'] == $field) {
+          foreach ($this->_pseudoConstantsSelect as $key => $pseudoConstantMetadata) {
+            // By replacing the join to the option value table with the mysql construct
+            // ORDER BY field('contribution_status_id', 2,1,4)
+            // we can remove a join. In the case of the option value join it is
+            /// a join known to cause slow queries.
+            // @todo cover other pseudoconstant types. Limited to option group ones in the
+            // first instance for scope reasons. They require slightly different handling as the column (label)
+            // is not declared for them.
+            // @todo so far only integer fields are being handled. If we add string fields we need to look at
+            // escaping.
+            if (isset($pseudoConstantMetadata['pseudoconstant'])
+              && isset($pseudoConstantMetadata['pseudoconstant']['optionGroupName'])
+              && $field === CRM_Utils_Array::value('optionGroupName', $pseudoConstantMetadata['pseudoconstant'])
+            ) {
+              $sortedOptions = $pseudoConstantMetadata['bao']::buildOptions($pseudoConstantMetadata['pseudoField'], NULL, array(
+                'orderColumn' => 'label',
+              ));
+              $order = str_replace("$field $direction", "field({$pseudoConstantMetadata['pseudoField']}," . implode(',', array_keys($sortedOptions)) . ") $direction", $order);
+            }
+            //CRM-12565 add "`" around $field if it is a pseudo constant
+            // This appears to be for 'special' fields like locations with appended numbers or hyphens .. maybe.
+            if (!empty($pseudoConstantMetadata['element']) && $pseudoConstantMetadata['element'] == $field) {
               $order = str_replace($field, "`{$field}`", $order);
             }
           }
       }
-      $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode);
-      $this->_simpleFromClause = self::fromClause($this->_whereTables, NULL, NULL, $this->_primaryLocation, $this->_mode);
     }
 
+    $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode);
+    $this->_simpleFromClause = self::fromClause($this->_whereTables, NULL, NULL, $this->_primaryLocation, $this->_mode);
+
     // The above code relies on crazy brittle string manipulation of a peculiarly-encoded ORDER BY
     // clause. But this magic helper which forgivingly reescapes ORDER BY.
     // Note: $sortByChar implies that $order was hard-coded/trusted, so it can do funky things.
-    if ($order && !$sortByChar) {
+    if ($sortByChar) {
+      return array(' ORDER BY ' . $order, $additionalFromClause);
+    }
+    if ($order) {
       $order = CRM_Utils_Type::escape($order, 'MysqlOrderBy');
       return array(' ORDER BY ' . $order, $additionalFromClause);
     }
-    return array($order, $additionalFromClause);
   }
 
   /**
@@ -6312,10 +6340,6 @@ private function pseudoConstantNameIsInReturnProperties($field) {
     if (!isset($field['pseudoconstant']['optionGroupName'])) {
       return FALSE;
     }
-    if (empty($field['bao']) || $field['bao'] != 'CRM_Contact_BAO_Contact') {
-      // For now....
-      return FALSE;
-    }
 
     if (CRM_Utils_Array::value($field['pseudoconstant']['optionGroupName'], $this->_returnProperties)) {
       return TRUE;

CRM/Contact/Selector.php

@@ -488,6 +488,9 @@ public function &getColumnHeaders($action = NULL, $output = NULL) {
         elseif (isset($this->_query->_fields[$prop]) && isset($this->_query->_fields[$prop]['title'])) {
           $title = $this->_query->_fields[$prop]['title'];
         }
+        elseif (isset($this->_query->_pseudoConstantsSelect[$prop]) && isset($this->_query->_pseudoConstantsSelect[$prop]['pseudoconstant']['optionGroupName'])) {
+          $title = CRM_Core_BAO_OptionGroup::getTitleByName($this->_query->_pseudoConstantsSelect[$prop]['pseudoconstant']['optionGroupName']);
+        }
         else {
           $title = '';
         }

CRM/Contribute/BAO/Contribution.php

@@ -780,6 +780,15 @@ public static function &importableFields($contactType = 'Individual', $status =
     return self::$_importableFields;
   }
 
+  /**
+   * Get exportable fields with pseudoconstants rendered as an extra field.
+   */
+  public static function getExportableFieldsWithPseudoConstants() {
+    $fields = self::exportableFields();
+    CRM_Core_DAO::appendPseudoConstantsToFields($fields);
+    return $fields;
+  }
+
   /**
    * Combine all the exportable fields from the lower level objects.
    *
@@ -799,14 +808,6 @@ public static function &exportableFields($checkPermission = TRUE) {
       $expFieldsContrib = CRM_Contribute_DAO_ContributionProduct::export();
       $typeField = CRM_Financial_DAO_FinancialType::export();
       $financialAccount = CRM_Financial_DAO_FinancialAccount::export();
-      $optionField = CRM_Core_OptionValue::getFields($mode = 'contribute');
-      $contributionStatus = array(
-        'contribution_status' => array(
-          'title' => ts('Contribution Status'),
-          'name' => 'contribution_status',
-          'data_type' => CRM_Utils_Type::T_STRING,
-        ),
-      );
 
       $contributionPage = array(
         'contribution_page' => array(
@@ -883,7 +884,7 @@ public static function &exportableFields($checkPermission = TRUE) {
         ),
       );
 
-      $fields = array_merge($impFields, $typeField, $contributionStatus, $contributionPage, $optionField, $expFieldProduct,
+      $fields = array_merge($impFields, $typeField, $contributionPage, $expFieldProduct,
         $expFieldsContrib, $contributionNote, $extraFields, $softCreditFields, $financialAccount, $premiums, $campaignTitle,
         CRM_Core_BAO_CustomField::getFieldsForImport('Contribution', FALSE, FALSE, FALSE, $checkPermission)
       );
@@ -1137,6 +1138,8 @@ public static function addPremium(&$params) {
    */
   public static function getContributionFields($addExtraFields = TRUE) {
     $contributionFields = CRM_Contribute_DAO_Contribution::export();
+    // @todo remove this - this line was added because payment_instrument_id was not
+    // set to exportable - but now it is.
     $contributionFields = array_merge($contributionFields, CRM_Core_OptionValue::getFields($mode = 'contribute'));
 
     if ($addExtraFields) {
@@ -3702,6 +3705,9 @@ public static function deleteContactContribution($contactId) {
   public static function buildOptions($fieldName, $context = NULL, $props = array()) {
     $className = __CLASS__;
     $params = array();
+    if (isset($props['orderColumn'])) {
+      $params['orderColumn'] = $props['orderColumn'];
+    }
     switch ($fieldName) {
       // This field is not part of this object but the api supports it
       case 'payment_processor':

CRM/Contribute/BAO/Query.php

@@ -97,39 +97,6 @@ public static function select(&$query) {
       $query->_tables['contribution_batch'] = 1;
     }
 
-    // get contribution_status
-    if (!empty($query->_returnProperties['contribution_status_id'])) {
-      $query->_select['contribution_status_id'] = "contribution_status.value as contribution_status_id";
-      $query->_element['contribution_status_id'] = 1;
-      $query->_tables['civicrm_contribution'] = 1;
-      $query->_tables['contribution_status'] = 1;
-    }
-
-    // get contribution_status label
-    if (!empty($query->_returnProperties['contribution_status'])) {
-      $query->_select['contribution_status'] = "contribution_status.label as contribution_status";
-      $query->_element['contribution_status'] = 1;
-      $query->_tables['civicrm_contribution'] = 1;
-      $query->_tables['contribution_status'] = 1;
-    }
-
-    // get payment instrument
-    if (!empty($query->_returnProperties['payment_instrument'])) {
-      $query->_select['payment_instrument'] = "contribution_payment_instrument.label as payment_instrument";
-      $query->_element['payment_instrument'] = 1;
-      $query->_tables['civicrm_contribution'] = 1;
-      $query->_tables['contribution_payment_instrument'] = 1;
-    }
-
-    // get payment instrument id
-    if (!empty($query->_returnProperties['payment_instrument_id'])) {
-      $query->_select['instrument_id'] = "contribution_payment_instrument.value as instrument_id";
-      $query->_select['payment_instrument_id'] = "contribution_payment_instrument.value as payment_instrument_id";
-      $query->_element['instrument_id'] = $query->_element['payment_instrument_id'] = 1;
-      $query->_tables['civicrm_contribution'] = 1;
-      $query->_tables['contribution_payment_instrument'] = 1;
-    }
-
     if (!empty($query->_returnProperties['contribution_campaign_title'])) {
       $query->_select['contribution_campaign_title'] = "civicrm_campaign.title as contribution_campaign_title";
       $query->_element['contribution_campaign_title'] = $query->_tables['civicrm_campaign'] = 1;
@@ -181,7 +148,7 @@ public static function whereClauseSingle(&$values, &$query) {
     list($name, $op, $value, $grouping, $wildcard) = $values;
 
     $quoteValue = NULL;
-    $fields = array_merge(CRM_Contribute_BAO_Contribution::fields(), self::getFields());
+    $fields = self::getFields();
 
     if (!empty($value) && !is_array($value)) {
       $quoteValue = "\"$value\"";
@@ -578,18 +545,6 @@ public static function from($name, $mode, $side) {
         $from .= " $side  JOIN civicrm_product ON civicrm_contribution_product.product_id =civicrm_product.id ";
         break;
 
-      case 'contribution_payment_instrument':
-        $from = " $side JOIN civicrm_option_group option_group_payment_instrument ON ( option_group_payment_instrument.name = 'payment_instrument')";
-        $from .= " $side JOIN civicrm_option_value contribution_payment_instrument ON (civicrm_contribution.payment_instrument_id = contribution_payment_instrument.value
-                               AND option_group_payment_instrument.id = contribution_payment_instrument.option_group_id ) ";
-        break;
-
-      case 'contribution_status':
-        $from = " $side JOIN civicrm_option_group option_group_contribution_status ON (option_group_contribution_status.name = 'contribution_status')";
-        $from .= " $side JOIN civicrm_option_value contribution_status ON (civicrm_contribution.contribution_status_id = contribution_status.value
-                               AND option_group_contribution_status.id = contribution_status.option_group_id ) ";
-        break;
-
       case 'contribution_softcredit_type':
         $from = " $side JOIN civicrm_option_group option_group_contribution_softcredit_type ON
           (option_group_contribution_softcredit_type.name = 'soft_credit_type')";

CRM/Contribute/DAO/Contribution.php

@@ -30,7 +30,7 @@
  *
  * Generated from xml/schema/CRM/Contribute/Contribution.xml
  * DO NOT EDIT.  Generated by CRM_Core_CodeGen
- * (GenCodeChecksum:b25399b3b6cf17e97809102666de8ee7)
+ * (GenCodeChecksum:3e387ab5c998da1bbb367ed526e84289)
  */
 require_once 'CRM/Core/DAO.php';
 require_once 'CRM/Utils/Type.php';
@@ -334,8 +334,12 @@ static function &fields() {
         'payment_instrument_id' => array(
           'name' => 'payment_instrument_id',
           'type' => CRM_Utils_Type::T_INT,
-          'title' => ts('Payment Method') ,
+          'title' => ts('Payment Method ID') ,
           'description' => 'FK to Payment Instrument',
+          'export' => true,
+          'where' => 'civicrm_contribution.payment_instrument_id',
+          'headerPattern' => '/^payment|(p(ayment\s)?instrument)$/i',
+          'dataPattern' => '',
           'table_name' => 'civicrm_contribution',
           'entity' => 'Contribution',
           'bao' => 'CRM_Contribute_BAO_Contribution',

CRM/Contribute/Selector/Search.php

@@ -366,6 +366,7 @@ public function &getRows($action, $offset, $rowCount, $sort, $output = NULL) {
     $allCampaigns = CRM_Campaign_BAO_Campaign::getCampaigns(NULL, NULL, FALSE, FALSE, FALSE, TRUE);
 
     while ($result->fetch()) {
+      $this->_query->convertToPseudoNames($result);
       $links = self::links($componentId,
           $componentAction,
           $qfKey,

CRM/Core/BAO/Mapping.php

@@ -381,7 +381,7 @@ public static function buildMappingForm(&$form, $mappingType, $mappingId, $colum
 
     if (($mappingType == 'Search Builder') || ($exportMode == CRM_Export_Form_Select::CONTRIBUTE_EXPORT)) {
       if (CRM_Core_Permission::access('CiviContribute')) {
-        $fields['Contribution'] = CRM_Contribute_BAO_Contribution::exportableFields();
+        $fields['Contribution'] = CRM_Contribute_BAO_Contribution::getExportableFieldsWithPseudoConstants();
         unset($fields['Contribution']['contribution_contact_id']);
         $compArray['Contribution'] = ts('Contribution');
       }

CRM/Core/BAO/OptionGroup.php

@@ -184,4 +184,39 @@ public static function ensureOptionGroupExists($params) {
     }
   }
 
+  /**
+   * Get the title of an option group by name.
+   *
+   * @param string $name
+   *   The name value for the option group table.
+   *
+   * @return string
+   *   The relevant title.
+   */
+  public static function getTitleByName($name) {
+    $groups = self::getTitlesByNames();
+    return $groups[$name];
+  }
+
+  /**
+   * Get a cached mapping of all group titles indexed by their unique name.
+   *
+   * We tend to only have a limited number of option groups so memory caching
+   * makes more sense than multiple look-ups.
+   *
+   * @return array
+   *   Array of all group titles by name.
+   *   e.g
+   *   array('activity_status' => 'Activity Status', 'msg_mode' => 'Message Mode'....)
+   */
+  public static function getTitlesByNames() {
+    if (!isset(\Civi::$statics[__CLASS__]) || !isset(\Civi::$statics[__CLASS__]['titles_by_name'])) {
+      $dao = CRM_Core_DAO::executeQuery("SELECT name, title FROM civicrm_option_group");
+      while ($dao->fetch()) {
+        \Civi::$statics[__CLASS__]['titles_by_name'][$dao->name] = $dao->title;
+      }
+    }
+    return \Civi::$statics[__CLASS__]['titles_by_name'];
+  }
+
 }

CRM/Core/DAO.php

@@ -2154,6 +2154,30 @@ public static function getGlobalSetting($name, $default = NULL) {
     }
   }
 
+
+  /**
+   * Update the fields array to also hold keys for pseudoconstant fields that relate to contained fields.
+   *
+   * This is relevant where we want to offer both the ID field and the label field
+   * as an option, e.g. search builder.
+   *
+   * It is currently limited for optionGroupName for purposes keeping the scope of the
+   * change small, but is appropriate for other sorts of pseudoconstants.
+   *
+   * @param array $fields
+   */
+  protected static function appendPseudoConstantsToFields(&$fields) {
+    foreach ($fields as $field) {
+      if (!empty($field['pseudoconstant']) && !empty($field['pseudoconstant']['optionGroupName'])) {
+        $fields[$field['pseudoconstant']['optionGroupName']] = array(
+          'title' => CRM_Core_BAO_OptionGroup::getTitleByName($field['pseudoconstant']['optionGroupName']),
+          'name' => $field['pseudoconstant']['optionGroupName'],
+          'data_type' => CRM_Utils_Type::T_STRING,
+        );
+      }
+    }
+  }
+
   /**
    * Get options for the called BAO object's field.
    *

CRM/Core/DAO/AllCoreTables.data.php

@@ -24,7 +24,7 @@
 | see the CiviCRM license FAQ at http://civicrm.org/licensing        |
 +--------------------------------------------------------------------+
 */
-// (GenCodeChecksum:5a41993a5c97a2544f3cc2e4df8879e2)
+// (GenCodeChecksum:b66a5c6ab79bec53c7781738c148e525)
 return array(
   'CRM_Core_DAO_AddressFormat' => array(
     'name' => 'AddressFormat',

CRM/Utils/Rule.php

@@ -138,6 +138,21 @@ public static function mysqlOrderByDirection($str) {
    * @return bool
    */
   public static function mysqlOrderBy($str) {
+    $matches = array();
+    // Using the field function in order by is valid.
+    // Look for a string like field(contribution_status_id,3,4,6).
+    // or field(civicrm_contribution.contribution_status_id,3,4,6)
+    if (preg_match('/field\([a-z_.]+,[0-9,]+\)/', $str, $matches)) {
+      // We have checked these. Remove them as they will fail the next lot.
+      // Our check currently only permits numbers & no back ticks. If we get a
+      // need for strings or backticks we can add.
+      $str = str_replace($matches, '', $str);
+    }
+    $str = trim($str);
+    if (!empty($matches) && empty($str)) {
+      // nothing left to check after the field check.
+      return TRUE;
+    }
     // Making a regex for a comma separated list is quite hard and not readable
     // at all, so we split and loop over.
     $parts = explode(',', $str);