Helper::formatAmount silently sets Amount to Zero about sagepay HOT 9 CLOSED

Yes, an exception would be better.

I'm not even sure if is_numeric() cuts it. We want to be sure that whatever is passed in, can be converted to a float, because that is what number_format() will be expecting. Casting does not throw errors in all cases:

echo (float)"123.456helloworld";
123.456

echo (float)"helloworld";
0

echo (float)"123,456.789";
0

What can we do about those?

from sagepay.

This is the kind of thing that worries me:

0xabc is a hexadecimal number. is_numeric agrees, if this is passed in as a string:

echo (is_numeric('0xabc') ? 'yes' : 'no');
yes

However, cast that string to a float, and we get a different result:

echo (float)'0xabc';
0

from sagepay.

Oh PHP!

I think it's reasonable to impose the same restrictions Sage does. Wikipedia suggests there are only two countries using non decimal currency and that they don't use the sub-unit. Anybody playing with currency in Hexidecimal either knows what they're doing or is nuts.

This is what I did in the Validator, and in my own code I stripped out everything which wasn't [0-9.] before passing it to SagePay. From my days working in a foreign exchange house I used to have a wonderful function for detecting using . as a thousand seperator and , for decimal, but I'm afraid it may have been lost in time.

    /**
    *   An invalid amount gets automatically turned into 0 when you set it, so we need to validate seperately
    **/
    public function validateAmount($amount) {
        // Check the Amount is a valid format
        if ( ! v::regex('/^[0-9,]+(\.[0-9]{1,2})?$/')->validate($amount)) {
            $this->addError('Amount', $this->AMOUNT_BAD_FORMAT);
            return $this;
        } else if (v::regex('/,[0-9]{2}$/')->validate($amount)) {
            // Detect possible use of comma as decimal delimiter
            $this->addError('Amount', $this->AMOUNT_BAD_FORMAT);
            return $this;
        }

        // Strip the Amount of commas so we can play with it as a number
        $amount = preg_replace('/[^0-9\.]/', '', $amount);
        // Lets work in pennies, because then we can't get confused with floats
        $amount = $amount * 100;
        if ( $amount < 1 || $amount > 10000000) {
            $this->addError('Amount', $this->AMOUNT_BAD_RANGE);
        }
        return $this;
    }

from sagepay.

Okay, we will limit currency to decimal digits, so we don't encourage crazy.

What I think we can't do, is assume the input currency has 100 pennies to the unit (some have zero and some have 1000). The range check would then apply to anything to the left of the decimal point, regardless of how many digits there are to the right of the decimal point.

I'm not sure what the SagePay range check applies to - probably to the integer part of the supplied currency.

Your check my work if all the hard-coded values (x100, i.e. 10^2), [0-9]{2} comes from the currency metadata, so it is relevant to the currency chosen.

Personally, just checking it contains digits and one decimal point (taking a . or a ,) as the decimal point would be enough for a validation check. Then it can be split() into integer and decimal parts, with range checking that can be done on those parts, with some care on leading/trailing zeroes.

Are you finding that amounts are provided that include thousands separators as well as a decimal point? Or is this something that would you like like to support just in case?

Where I am coming from, is that I would like to support whatever currency the application uses, and that goes through to the SagePay API. Having tried out other currencies, I realise the "two decimal places" for amounts sent to the API in the documentation is wrong; the currency must be formatted according to the ISO standard for that currency, or SagePay will reject it.

Bot sure if that helps or hinders? One saving grace is that if we get the validation wrong, and let some edge-cases through, SagePay will still catch it for us.

from sagepay.

The system I've setup to use this is going to be used by 5 people as the back-end for their organisation. So I'm allowed to say "enter it this way", but I know from creating the "Your Approximate Requirements" section of this that it's not that simple usually. Try entering '1.234,33' and see how it auto-corrects after you submit.

I'd suggest we want a test which has many valid currency formats and then write something which works with them all.

from sagepay.

filter_var() looks neat. Here are a few tests:

http://www.acadweb.co.uk/tests/

It would need to be run [up to] twice - once with a dot as the decimal point and once as a comma. It handles leading and trailing zeroes and also an optional thousands separator. If we just relied on the locale for the decimal point, then it would only need to be run once. This will spit out a double value if it passes the validation.

The double can then be passed through number_format() with the appropriate decimal places for the currency. The value of that should not change, and if it doe, that indicates a decimal range error (123.456 != 123.46 to number_format of 2 dps).

The validated double must be greater than zero.

The validated double can be floored to an integer, and that would give us a value to check against the maximum range that SagePay will accept (taking into account what the decimal part may contain, maybe).

How does that sound? In summary:

1. $validated = filter_var($value, FILTER_VALIDATE_FLOAT) - with locale dp or try both, and with thousands separator flag set.
2. If $validated is false, then invalid.
3. $validated2 = (double)number_format($validated, $currency_dp_size)
4. If $validated != $validated2 then decimal part is out of range.
5. $integer_part = floor($validated2).
6. If $integer_part is too high then validation fails (not got docs handy, so not sure if max range is all the 9s).

I'll expand my test script to try the full thing, and see how it goes. The thousands separator could be a problem - is 1,001 a thousand and one, or one and a thousandth? Only falling back to the locale to tell us, or at least to tell us which interpretation to try validating against first, is going to be reliable.

from sagepay.

I'll work to trying to get this implemented in an i18n flexible way, with a wide variety of formats accepted, so whatever stricter validation rules you apply in the application, it should still be accepted.

from sagepay.

Looking at this further, I cannot find any payment gateways or accountancy packages that use commas for the decimal separator. PHP uses the full stop or period (.) in all its numeric operations. Numbers are always represented as decimals like this (e.g. 1.23). Like most - or all - PHP payment gateways, we are going to enforce that kind of decimal point, with optional comma (,) thousands separators, if passed in as a string.

from sagepay.

Ideally we would use currency and amount objects to handle the amount. The successor to this package does. For now, I've put a validation check in the point where the currency is formatted. If passed in as a string, the format must adhere to:

• Always start with a digit (not "," or ".")
• Use a single but optional decimal point (.)
• The decimal digits must not exceed the minor units for the currency. So GDP 1.00 is allowed, but GDP 1.000 is not, even if they have the same decimal value.
• Optional thousands separators use the comma (,)

from sagepay.