Issue #17 better validation of amount values.

Author: judgej

src/Omnipay/Common/Message/AbstractRequest.php

@@ -161,14 +161,40 @@ public function setCardReference($value)
     public function getAmount()
     {
         $amount = $this->getParameter('amount');
-        if ($amount) {
-            if (!is_float($amount) &&
-                $this->getCurrencyDecimalPlaces() > 0 &&
-                false === strpos((string) $amount, '.')) {
-                throw new InvalidRequestException(
-                    'Please specify amount as a string or float, ' .
-                    'with decimal places (e.g. \'10.00\' to represent $10.00).'
-                );
+        $message = 'Please specify amount as a string or float, '
+            . 'with decimal places (e.g. \'10.00\' to represent $10.00).';
+
+        if (isset($amount)) {
+            // Don't allow integers for currencies that support decimals.
+            // This is for legacy reasons - upgrades from v0.9
+            if (is_int($amount) && $this->getCurrencyDecimalPlaces() > 0) {
+                throw new InvalidRequestException($message);
+            }
+
+            if (is_string($amount)) {
+                // A '-' is not considered a valid character, so a negative amounts will be invalid.
+                if (preg_match('/[^0-9\.]/', $amount)) {
+                    throw new InvalidRequestException('Invalid character in amount.');
+                }
+
+                // Generic number, with optional decimals.
+                if (!preg_match('/^[0-9]+(\.[0-9]*)?$/', $amount)) {
+                    throw new InvalidRequestException('Amount string is not a valid decimal number.');
+                }
+
+                // Don't allow integers for currencies that support decimals (legacy v0.9).
+                if ($this->getCurrencyDecimalPlaces() > 0 && false === strpos((string) $amount, '.')) {
+                    throw new InvalidRequestException($message);
+                }
+            }
+
+            // The number_format() used later requires a float.
+            $amount = (float)$amount;
+
+            // Check for rounding that may occur if too many decimal places are supplied.
+            $decimal_count = strlen(substr(strrchr((string)$amount, '.'), 1));
+            if ($decimal_count > $this->getCurrencyDecimalPlaces()) {
+                throw new InvalidRequestException('Amount precision is too high for currency.');
             }
 
             return $this->formatCurrency($amount);

tests/Omnipay/Common/Message/AbstractRequestTest.php

@@ -87,18 +87,28 @@ public function testAmountWithEmpty()
         $this->assertSame(null, $this->request->getAmount());
     }
 
+    public function testAmountZero()
+    {
+        $this->assertSame($this->request, $this->request->setAmount(0.0));
+        $this->assertSame('0.00', $this->request->getAmount());
+    }
+
     public function testGetAmountNoDecimals()
     {
         $this->assertSame($this->request, $this->request->setCurrency('JPY'));
         $this->assertSame($this->request, $this->request->setAmount('1366'));
         $this->assertSame('1366', $this->request->getAmount());
     }
 
+    /**
+     * @expectedException Omnipay\Common\Exception\InvalidRequestException
+     */
     public function testGetAmountNoDecimalsRounding()
     {
+        // There will not be any rounding; the amount is sent as requested or not at all.
         $this->assertSame($this->request, $this->request->setAmount('136.5'));
         $this->assertSame($this->request, $this->request->setCurrency('JPY'));
-        $this->assertSame('137', $this->request->getAmount());
+        $this->request->getAmount();
     }
 
     /**
@@ -117,6 +127,7 @@ public function testAmountWithIntThrowsException()
     public function testAmountWithIntStringThrowsException()
     {
         // ambiguous value, avoid errors upgrading from v0.9
+        // Some currencies only take integers, so an integer (in a string) should be valid.
         $this->assertSame($this->request, $this->request->setAmount('10'));
         $this->request->getAmount();
     }
@@ -134,6 +145,24 @@ public function testGetAmountIntegerNoDecimals()
         $this->assertSame(1366, $this->request->getAmountInteger());
     }
 
+    /**
+     * @expectedException Omnipay\Common\Exception\InvalidRequestException
+     */
+    public function testAmountThousandsSepThrowsException()
+    {
+        $this->assertSame($this->request, $this->request->setAmount('1,234.00'));
+        $this->request->getAmount();
+    }
+
+    /**
+     * @expectedException Omnipay\Common\Exception\InvalidRequestException
+     */
+    public function testAmountInvalidFormatThrowsException()
+    {
+        $this->assertSame($this->request, $this->request->setAmount('1.234.00'));
+        $this->request->getAmount();
+    }
+
     public function testCurrency()
     {
         $this->assertSame($this->request, $this->request->setCurrency('USD'));