CRM-16819 improve on CRM_Utils_Request::retrieve
authoreileen <emcnaughton@wikimedia.org>
Wed, 15 Nov 2017 23:42:07 +0000 (12:42 +1300)
committereileen <emcnaughton@wikimedia.org>
Thu, 16 Nov 2017 00:26:09 +0000 (13:26 +1300)
I'm proposing this as an improved (but not perfect) way to retrieve data from http requests.

The biggest issue IMHO with the CRM_Utils_Request::retrieve method is that the third
parameter is & - and if any of the subsequent parameters need to be passed a NULL
array must be generated. The practice of passing a specific static NULL array is known to
have caused errors due to contamination. This commit offers an alternate function without that
parameter (my suggestion is to create a new function again where we really do want  to
work but in general it seems flakey to me). I have also re-ordered the parameters as I perceive
 as being more commonly required than  (renamed from 'abort').

I have also made it so the new function will throw an exception rather than use the deprecated
fatal - as a new function I feel we can enforce this change and I have made some changes
to the error message thrown in the validate function so it returns a list of permissable
values for the  string as the existing error is hard for developers to work with, as it
can be hard to realise why 'int' is wrong (casing) or what should have been passed.

Finally I made the visibility on CRM_Utils_Retrieve::getValues protected. This function
is not called from within core & is downright unsafe as it does no validation so we should
not permit anyone to accidentally use it (I have no reason to think they have

CRM/Contact/Page/DedupeMerge.php
CRM/Utils/Request.php
CRM/Utils/Type.php

index 7cd3776acbaab37898116c92c0ecf5776c42da89..d3cc7998545e3750b6193bb5ab023069964ba038 100644 (file)
@@ -40,14 +40,11 @@ class CRM_Contact_Page_DedupeMerge extends CRM_Core_Page {
   public function run() {
     $runner = self::getRunner();
     if ($runner) {
-      // Run Everything in the Queue via the Web.
       $runner->runAllViaWeb();
     }
     else {
       CRM_Core_Session::setStatus(ts('Nothing to merge.'));
     }
-
-    // parent run
     return parent::run();
   }
 
@@ -55,11 +52,11 @@ class CRM_Contact_Page_DedupeMerge extends CRM_Core_Page {
    * Build a queue of tasks by dividing dupe pairs in batches.
    */
   public static function getRunner() {
-    $rgid = CRM_Utils_Request::retrieve('rgid', 'Positive');
-    $gid  = CRM_Utils_Request::retrieve('gid', 'Positive');
-    $limit  = CRM_Utils_Request::retrieve('limit', 'Positive');
-    $action = CRM_Utils_Request::retrieve('action', 'String');
-    $mode   = CRM_Utils_Request::retrieve('mode', 'String', CRM_Core_DAO::$_nullObject, FALSE, 'safe');
+    $rgid = CRM_Utils_Request::retrieveValue('rgid', 'Positive');
+    $gid  = CRM_Utils_Request::retrieveValue('gid', 'Positive');
+    $limit = CRM_Utils_Request::retrieveValue('limit', 'Positive');
+    $action = CRM_Utils_Request::retrieveValue('action', 'String');
+    $mode = CRM_Utils_Request::retrieveValue('mode', 'String', 'safe');
 
     $cacheKeyString = CRM_Dedupe_Merger::getMergeCacheKeyString($rgid, $gid);
 
@@ -68,7 +65,7 @@ class CRM_Contact_Page_DedupeMerge extends CRM_Core_Page {
       'action' => 'update',
       'rgid' => $rgid,
       'gid' => $gid,
-      'limit' => $limit
+      'limit' => $limit,
     );
 
     if ($mode == 'aggressive' && !CRM_Core_Permission::check('force merge duplicate contacts')) {
index 2748e07d7df8e2c16d361009ff96d2e1a6f7ae9e..ad6bf094e593f233cd3732b32a072ca3a282c715 100644 (file)
@@ -79,17 +79,15 @@ class CRM_Utils_Request {
    *   Default value of the variable if not present.
    * @param string $method
    *   Where to look for the variable - 'GET', 'POST' or 'REQUEST'.
+   * @param bool $isThrowException
+   *   Should a an exception be thrown rather than a fatal.
    *
    * @return mixed
    *   The value of the variable
+   *
+   * @throws \CRM_Core_Exception
    */
-  public static function retrieve($name, $type, &$store = NULL, $abort = FALSE, $default = NULL, $method = 'REQUEST') {
-
-    // hack to detect stuff not yet converted to new style
-    if (!is_string($type)) {
-      CRM_Core_Error::backtrace();
-      CRM_Core_Error::fatal(ts("Please convert retrieve call to use new function signature"));
-    }
+  public static function retrieve($name, $type, &$store = NULL, $abort = FALSE, $default = NULL, $method = 'REQUEST', $isThrowException = FALSE) {
 
     $value = NULL;
     switch ($method) {
@@ -117,6 +115,9 @@ class CRM_Utils_Request {
     }
 
     if (!isset($value) && $abort) {
+      if ($isThrowException) {
+        throw new CRM_Core_Exception(ts("Could not find valid value for %1", array(1 => $name)));
+      }
       CRM_Core_Error::fatal(ts("Could not find valid value for %1", array(1 => $name)));
     }
 
@@ -145,7 +146,7 @@ class CRM_Utils_Request {
    * @return mixed
    *    The value of the variable
    */
-  public static function getValue($name, $method) {
+  protected static function getValue($name, $method) {
     if (isset($method[$name])) {
       return $method[$name];
     }
@@ -165,6 +166,10 @@ class CRM_Utils_Request {
   }
 
   /**
+   * @deprecated
+   *
+   * We should use a function that checks url values.
+   *
    * This is a replacement for $_REQUEST which includes $_GET/$_POST
    * but excludes $_COOKIE / $_ENV / $_SERVER.
    *
@@ -186,4 +191,32 @@ class CRM_Utils_Request {
     return $result;
   }
 
+  /**
+   * Retrieve a variable from the http request.
+   *
+   * @param string $name
+   *   Name of the variable to be retrieved.
+   * @param string $type
+   *   Type of the variable (see CRM_Utils_Type for details).
+   *   Most common options are:
+   *   - 'Integer'
+   *   - 'Positive'
+   *   - 'CommaSeparatedIntegers'
+   *   - 'Boolean'
+   *   - 'String'
+   *
+   * @param mixed $defaultValue
+   *   Default value of the variable if not present.
+   * @param bool $isRequired
+   *   Is the variable required for this function to proceed without an exception.
+   * @param string $method
+   *   Where to look for the value - GET|POST|REQUEST
+   *
+   * @return mixed
+   */
+  public static function retrieveValue($name, $type, $defaultValue = NULL, $isRequired = FALSE, $method = 'REQUEST') {
+    $null = NULL;
+    return CRM_Utils_Request::retrieve((string) $name, (string) $type, $null, (bool) $isRequired, $defaultValue, $method, TRUE);
+  }
+
 }
index b10fbc891621a1733a0d3b0ec759431e72590587..29b05579aec49e98fe344da20a966fad9a636fcb 100644 (file)
@@ -391,13 +391,44 @@ class CRM_Utils_Type {
    *   The type to validate against.
    * @param bool $abort
    *   If TRUE, the operation will CRM_Core_Error::fatal() on invalid data.
-   * @name string $name
+   * @param string $name
    *   The name of the attribute
+   * @param bool $isThrowException
+   *   Should an exception be thrown rather than a using a deprecated fatal error.
    *
    * @return mixed
    *   The data, escaped if necessary
+   *
+   * @throws \CRM_Core_Exception
    */
-  public static function validate($data, $type, $abort = TRUE, $name = 'One of parameters ') {
+  public static function validate($data, $type, $abort = TRUE, $name = 'One of parameters ', $isThrowException = FALSE) {
+
+    $possibleTypes = array(
+      'Integer',
+      'Int',
+      'Positive',
+      'CommaSeparatedIntegers',
+      'Boolean',
+      'Float',
+      'Money',
+      'Text',
+      'String',
+      'Link',
+      'Memo',
+      'Date',
+      'Timestamp',
+      'ContactReference',
+      'MysqlColumnNameOrAlias',
+      'MysqlOrderByDirection',
+      'MysqlOrderBy',
+      'ExtensionKey',
+    );
+    if (!in_array($type, $possibleTypes)) {
+      if ($isThrowException) {
+        throw new CRM_Core_Exception(ts('Invalid type, must be one of : ' . implode($possibleTypes)));
+      }
+      CRM_Core_Error::fatal(ts('Invalid type, must be one of : ' . implode($possibleTypes)));
+    }
     switch ($type) {
       case 'Integer':
       case 'Int':
@@ -499,14 +530,13 @@ class CRM_Utils_Type {
           return $data;
         }
         break;
-
-      default:
-        CRM_Core_Error::fatal("Cannot recognize $type for $data");
-        break;
     }
 
     if ($abort) {
       $data = htmlentities($data);
+      if ($isThrowException) {
+        throw new CRM_Core_Exception("$name (value: $data) is not of the type $type");
+      }
       CRM_Core_Error::fatal("$name (value: $data) is not of the type $type");
     }