Improve type checking in getContactPhone and use CRM_Utils_Request::r…

[seamuslee001]

…etrieve to get data from GET

Overview

This improves the type checking on the name key by making it use the type Alphanumeric rather than string. It also switches to use CRM_Utils_Request::retrieve rather than CRM_Utils_Array::value to get data from the $_GET variable

ping @eileenmcnaughton @monishdeb @colemanw

[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]

I tested with

http://dmaster.local/civicrm/ajax/checkphone?cid=96,97,95,148

http://dmaster.local/civicrm/ajax/checkphone?name=Jacobs, Elbert

& this didn't work. For the first url the following patch worked

:...skipping...
diff --git a/CRM/Contact/Page/AJAX.php b/CRM/Contact/Page/AJAX.php
index 8a4ffdf6e8..24ffa88ae2 100644
--- a/CRM/Contact/Page/AJAX.php
+++ b/CRM/Contact/Page/AJAX.php
@@ -472,7 +472,7 @@ LIMIT {$offset}, {$rowCount}
       $queryString = " ( cc.sort_name LIKE '%$name%' OR cp.phone LIKE '%$name%' ) ";
     }
     else {
-      $cid = CRM_Utils_Request::retrieve('cid', 'CommaSeparatedIntegers', NULL, FALSE, NULL, 'GET');
+      $cid = CRM_Utils_Request::retrieveValue('cid', 'CommaSeparatedIntegers', NULL, FALSE, 'GET');
       if ($cid) {
         $queryString = " cc.id IN ( $cid )";
       }
@@ -480,9 +480,8 @@ LIMIT {$offset}, {$rowCount}
 
     if ($queryString) {
       $result = array();
-      $offset = CRM_Utils_Request::retrieve('offset', 'Integer', NULL, FALSE, 0, 'GET');
-      $rowCount = CRM_Utils_Request::retrieve('rowcount', 'Integer', NULL, FALSE, 20, 'GET');
-
+      $offset = (int) CRM_Utils_Request::retrieveValue('offset', 'Integer', 0, FALSE, 'GET');
+      $rowCount = (int) CRM_Utils_Request::retrieveValue('rowcount', 'Integer', 20, FALSE, 'GET');
       // add acl clause here
       list($aclFrom, $aclWhere) = CRM_Contact_BAO_Contact_Permission::cacheClause('cc');
       if ($aclWhere) {

Note that the (int) is required because of the way 0 is handled in retrieve function

For the 'name' retrieval it's more complex. Valid name variables include more than just alphanumeric. In my test there was a comma and a space (it works off sort_name) but also consider O'Connor. We probably need to filter things out ? Do we need a new filter

[seamuslee001]

@eileenmcnaughton possibly yes, i guess what i can do is for the moment send it back to string but yeh i think it would be good. Maybe we could allow for one space in the alphanumeric but hmmm its a difficult question

[eileenmcnaughton]

@seamuslee001 specifically stripping < & > seems good

[colemanw]

I really don't think we should be concatenating string values from the url into a query. I think this line ought to be using %1 %2 etc placeholders and we should be feeding values into an array as the 2nd param to executeQuery.

[eileenmcnaughton]

@seamuslee001 I took a look at this - there are a few instance where you use retrieveValue instead of the existing CRM_Utils_Array::value + escape. For commaSeparatedInteger & Integer I think the validate action is solid enough not to also require escaping.

However for the first one - ie. the one that is string - the validate doesn't do much now we've backed off your original 'Alphanumeric' - making the change @colemanw suggested would work - although the way the hook has been jammed in that function it arguably could mess with it. Alternatively CRM_Core_DAO::composeQuery could be used to do the interpolation.

[eileenmcnaughton]

@seamuslee001 this needs an update by you per comments

[seamuslee001]

@eileenmcnaughton updated now

[eileenmcnaughton]

Thanks @seamuslee001 this is good to merge IMHO

[eileenmcnaughton]

@seamuslee001 oops I take it back - I'm getting a notice

[seamuslee001]

@eileenmcnaughton hopefully fixed that e-notice now

[eileenmcnaughton]

@seamuslee001 yep seems good

[seamuslee001]

Merging as per the tag