dev/core#748 Switch alphabetQuery to use new getSearchSQLParts() function

[mattwire]

Overview

This replaces #13733 following merge of #13735. Partial based on #13713

Before

getSearchSQL is only called from one place in with $sortByChar set to TRUE and each part of the query building functions have special handling for this parameter. We disable full group by for this query as it's not supported.

After

Some of the specific handling for $sortByChar query is pulled out of the generic functions, simplifying them. We generate the specific lines of query in the one place they are actually used. We also generate a GROUP BY clause that is compatible with fullgroupby.

Technical Details

$sortByChar is used only for a single, very specific query to generate the list of characters which should be displayed as dynamic filters at the top of search results. The code which builds the search queries is over-complex and this helps simplify it.

Comments

@eileenmcnaughton Replacement for #13733 based on your work in #13735

[civibot]

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

[eileenmcnaughton]

This looks good! The craziness of this legacy code is starting to dissapate

I'll pull it down & give it a spin once jenkins has passed judgement

[eileenmcnaughton]

Just noting the alphabet query does have test coverage

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

@mattwire I went through & did the same refactor as you & here is a patch of mine against yours

--- a/CRM/Contact/BAO/Query.php
+++ b/CRM/Contact/BAO/Query.php
@@ -4924,11 +4924,15 @@ civicrm_relationship.start_date > {$today}
    * @return CRM_Core_DAO
    */
   public function alphabetQuery() {
-    $sqlParts = $this->getSearchSQLParts(NULL, NULL, NULL, FALSE, FALSE, TRUE);
-    $sqlParts['group_by'] = 'GROUP BY sort_name';
-    $sqlParts['order_by'] = 'ORDER BY sort_name asc';
-    $query = "{$sqlParts['select']} {$sqlParts['from']} {$sqlParts['where']} {$sqlParts['having']} {$sqlParts['group_by']} {$sqlParts['order_by']}";
+    CRM_Core_DAO::disableFullGroupByMode();
 
+    $sqlParts = $this->getSearchSQLParts(NULL, NULL, NULL, FALSE, FALSE, TRUE);
+    $query = "SELECT DISTINCT LEFT(contact_a.sort_name, 1) as sort_name
+     {$sqlParts['from']}
+     {$sqlParts['where']}
+     {$sqlParts['having']}
+     GROUP BY sort_name
+     ORDER BY sort_name asc";
     $dao = CRM_Core_DAO::executeQuery($query);
 
     // We can always call this - it will only re-enable if it was originally enabled.
@@ -6636,6 +6640,10 @@ AND   displayRelType.is_active = 1
     $additionalFromClause = NULL, $skipOrderAndLimit = FALSE) {
 
     $sqlParts = $this->getSearchSQLParts($offset, $rowCount, $sort, $count, $includeContactIds, $sortByChar, $groupContacts, $additionalWhereClause, $sortOrder, $additionalFromClause);
+    if ($sortByChar) {
+      CRM_Core_Error::deprecatedFunctionWarning('sort by char is deprecated - use alphabetquery method');
+      $sqlParts['order_by'] = 'ORDER BY sort_name asc';
+    }
 

The important parts are

1. ensuring CRM_Core_DAO::disableFullGroupByMode(); is called from alphabet query
2. supporting but deprecating the $sortByChar function further up the chain - I'm worried about custom searches outside of core

Also note we can probably simplify the prepareOrderBy & general web further is we agree this can be merged #13748 - that $additionalFromClause is a pain from a code POV but I think it actually BREAKS stuff from a usability pov

[mattwire]

@eileenmcnaughton With the GROUP BY change we don't need to call disableFullGroupByMode() at all as the query is now compatible with full group by

[eileenmcnaughton]

@mattwire ah yeah - you are right re FGB - so we should remove the reenable as well I think. I do think the other part I suggested - the deprecation rather than remove from parent could be needed for custom searches

[mattwire]

@eileenmcnaughton I think I've addressed your comments. If tests pass this is ready for review again

[mattwire]

@eileenmcnaughton Ok I've addressed your comments - I think this is now ok to merge?

[eileenmcnaughton]

@mattwire those deprecated lines are hit... :-(

[mattwire]

It's catch 22. Leave the $sortByChar ifs' in and you do some of the work twice because if you don't pass in $sortByChar=TRUE you trigger the query functions to do a load of other work. If you take out $sortByChar then you risk breaking custom searches that may have been implemented by third-parties IF they call getSearchSQL with sortByChar=TRUE. Which was only called once by core via PagerAToZ.php in getDynamicCharacters().

Could we just put a deprecated warning into getSearchSQL() and return a NULL query? The worst thing I think it's going to break is that you'll only have the standard letters across the top of the results instead of all "extra" letters. Which is not likely to affect English and most European names which start with a standard letter even if they then use accents etc. later on. And provide an upgrader message to the effect that you should check your custom searches if relying on this feature.

[eileenmcnaughton]

additionalFromClause is no longer required in this signature - I haven't removed to keep this from going stale but we should do as a follow up

[eileenmcnaughton]

unrelated fail