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 Conflicts: tools/scripts/git/commit-msg
CiviCRM42 · Oct 2, 2013 · f18140d · f18140d
1 parent 3edf128
commit f18140d
Show file tree

Hide file tree

Showing 3 changed files with 163 additions and 6 deletions.
diff --git 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 @@ static function integer($value) {
       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,14 +301,25 @@ static function positiveInteger($value) {
       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;
     }
 
     return FALSE;
   }
 
   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
@@ -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
@@ -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),
+    );
+  }
+
+
+}