CRM-13460 - Add is_numeric checks and test coverage

----------------------------------------
* CRM-13460: Make the numeric rule checks stricter
  http://issues.civicrm.org/jira/browse/CRM-13460

diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php
index b6b6e8d4fb486affe7743e0ff86444d09c80d257..cadc93dc786b50caf7c86ce8e9b234ab8c2f551b 100644 (file)
--- a/CRM/Utils/Rule.php
+++ b/CRM/Utils/Rule.php
@@ -34,6 +34,7 @@
  */
 
 require_once 'HTML/QuickForm/Rule/Email.php';
+
 class CRM_Utils_Rule {
 
   static function title($str, $maxLength = 127) {
@@ -272,17 +273,26 @@ class CRM_Utils_Rule {
       return TRUE;
     }
 
-    if (($value < 0)) {
+    // CRM-13460
+    // ensure number passed is always a string numeral
+    if (!is_numeric($value)) {
+      return FALSE;
+    }
+
+    // note that is_int matches only integer type
+    // and not strings which are only integers
+    // hence we do this here
+    if (preg_match('/^\d+$/', $value)) {
+      return TRUE;
+    }
+
+    if ($value < 0) {
       $negValue = -1 * $value;
       if (is_int($negValue)) {
         return TRUE;
       }
     }
 
-    if (is_numeric($value) && preg_match('/^\d+$/', $value)) {
-      return TRUE;
-    }
-
     return FALSE;
   }
 
@@ -291,7 +301,13 @@ class CRM_Utils_Rule {
       return ($value < 0) ? FALSE : TRUE;
     }
 
-    if (is_numeric($value) && preg_match('/^\d+$/', $value)) {
+    // CRM-13460
+    // ensure number passed is always a string numeral
+    if (!is_numeric($value)) {
+      return FALSE;
+    }
+
+    if (preg_match('/^\d+$/', $value)) {
       return TRUE;
     }
 
@@ -299,6 +315,11 @@ class CRM_Utils_Rule {
   }
 
   static function numeric($value) {
+    // lets use a php gatekeeper to ensure this is numeric
+    if (!is_numeric($value)) {
+      return FALSE;
+    }
+
     return preg_match('/(^-?\d\d*\.\d*$)|(^-?\d\d*$)|(^-?\.\d\d*$)/', $value) ? TRUE : FALSE;
   }
 

diff --git a/tests/phpunit/CRM/Utils/RuleTest.php b/tests/phpunit/CRM/Utils/RuleTest.php
new file mode 100644 (file)
index 0000000..2207117
--- /dev/null
+++ b/tests/phpunit/CRM/Utils/RuleTest.php
@@ -0,0 +1,74 @@
+<?php
+
+require_once 'CiviTest/CiviUnitTestCase.php';
+
+class CRM_Utils_RuleTest extends CiviUnitTestCase {
+
+  function get_info() {
+    return array(
+      'name'      => 'Rule Test',
+      'description' => 'Test the validation rules',
+      'group'      => 'CiviCRM BAO Tests',
+    );
+  }
+
+  function setUp() {
+    parent::setUp();
+  }
+
+  /**
+   * @dataProvider integerDataProvider
+   */
+  function testInteger($inputData, $expectedResult) {
+    $this->assertEquals($expectedResult, CRM_Utils_Rule::integer($inputData));
+  }
+
+  function integerDataProvider() {
+    return array(
+      array(10, true),
+      array('145E+3', false),
+      array('10', true),
+      array(-10, true),
+      array('-10', true),
+      array('-10foo', false),
+    );
+  }
+
+  /**
+   * @dataProvider positiveDataProvider
+   */
+  function testPositive($inputData, $expectedResult) {
+    $this->assertEquals($expectedResult, CRM_Utils_Rule::positiveInteger($inputData, $inputType));
+  }
+
+  function positiveDataProvider() {
+    return array(
+      array(10, true),
+      array('145.0E+3', false),
+      array('10', true),
+      array(-10, false),
+      array('-10', false),
+      array('-10foo', false),
+    );
+  }
+
+  /**
+   * @dataProvider numericDataProvider
+   */
+  function testNumeric($inputData, $expectedResult) {
+    $this->assertEquals($expectedResult, CRM_Utils_Rule::numeric($inputData, $inputType));
+  }
+
+  function numericDataProvider() {
+    return array(
+      array(10, true),
+      array('145.0E+3', false),
+      array('10', true),
+      array(-10, true),
+      array('-10', true),
+      array('-10foo', false),
+    );
+  }
+
+
+}

diff --git a/tests/phpunit/CRM/Utils/TypeTest.php b/tests/phpunit/CRM/Utils/TypeTest.php
new file mode 100644 (file)
index 0000000..a034e40
--- /dev/null
+++ b/tests/phpunit/CRM/Utils/TypeTest.php
@@ -0,0 +1,62 @@
+<?php
+
+require_once 'CiviTest/CiviUnitTestCase.php';
+
+class CRM_Utils_TypeTest extends CiviUnitTestCase {
+
+  function get_info() {
+    return array(
+      'name'      => 'Type Test',
+      'description' => 'Test the validate function',
+      'group'      => 'CiviCRM BAO Tests',
+    );
+  }
+
+  function setUp() {
+    parent::setUp();
+  }
+
+  /**
+   * @dataProvider validateDataProvider
+   */
+  function testValidate($inputData, $inputType, $expectedResult) {
+    $this->assertEquals($expectedResult, CRM_Utils_Type::validate($inputData, $inputType, FALSE));
+  }
+
+  function validateDataProvider() {
+    return array(
+      array(10, 'Int', 10),
+      array('145E+3', 'Int', NULL),
+      array('10', 'Integer', 10),
+      array(-10, 'Int', -10),
+      array('-10', 'Integer', -10),
+      array('-10foo', 'Int', NULL),
+      array(10, 'Positive', 10),
+      array('145.0E+3', 'Positive', NULL),
+      array('10', 'Positive', 10),
+      array(-10, 'Positive', NULL),
+      array('-10', 'Positive', NULL),
+      array('-10foo', 'Positive', NULL),
+    );
+  }
+
+  /**
+   * @dataProvider numericDataProvider
+   */
+  function testNumeric($inputData, $expectedResult) {
+    $this->assertEquals($expectedResult, CRM_Utils_Rule::numeric($inputData, $inputType));
+  }
+
+  function numericDataProvider() {
+    return array(
+      array(10, true),
+      array('145.0E+3', false),
+      array('10', true),
+      array(-10, true),
+      array('-10', true),
+      array('-10foo', false),
+    );
+  }
+
+
+}

diff --git a/tools/scripts/git/commit-msg b/tools/scripts/git/commit-msg
index 93d0df693424dd83457ff0d67eafb427398f3ffe..1d364daca25a49c7325b1012744c714ee2296e94 100755 (executable)
--- a/tools/scripts/git/commit-msg
+++ b/tools/scripts/git/commit-msg
@@ -15,5 +15,5 @@ if [ -f "$PACKAGES/git-footnote/bin/git-footnote" ]; then
   TMPFILE="$1.jira"
   cp -p "$FILE" "$TMPFILE"
   $PACKAGES/git-footnote/bin/git-footnote CRM http://issues.civicrm.org/jira < "$TMPFILE" > "$FILE"
-  rm -f "$TMPFILE"
+  # rm -f "$TMPFILE"
 fi