From f18140d5b29f34441179f0e28d95e215d936de47 Mon Sep 17 00:00:00 2001
From: "Donald A. Lobo" <lobo@civicrm.org>
Date: Fri, 27 Sep 2013 14:01:13 -0700
Subject: [PATCH] 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
---
 CRM/Utils/Rule.php                   | 33 ++++++++++---
 tests/phpunit/CRM/Utils/RuleTest.php | 74 ++++++++++++++++++++++++++++
 tests/phpunit/CRM/Utils/TypeTest.php | 62 +++++++++++++++++++++++
 3 files changed, 163 insertions(+), 6 deletions(-)
 create mode 100644 tests/phpunit/CRM/Utils/RuleTest.php
 create mode 100644 tests/phpunit/CRM/Utils/TypeTest.php

diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php
index 21a07781..e17a6cc9 100644
--- 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,7 +301,13 @@ 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;
     }
 
@@ -299,6 +315,11 @@ static function positiveInteger($value) {
   }
 
   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
index 00000000..22071172
--- /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
index 00000000..a034e40f
--- /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),
+    );
+  }
+
+
+}