moneyphp / money

PHP implementation of Fowler's Money pattern.
http://moneyphp.org
MIT License
4.62k stars 440 forks source link

Fix: ISO parsing invalid assertions for CurrencyPair #698

Closed sudkumar closed 2 years ago

sudkumar commented 2 years ago

With iso as EUR/USD 1.2500, we will get $matches = ['EUR/USD 1.2500', 'EUR', 'USD', '1.2500'] after pref_match but the assertion expects $matches[2] to be numeric which seems to be incorrect. Just updated the assertion to not expect an empty string.

assert($matches[1] !== '');
- assert(is_numeric($matches[2]));
+ assert($matches[2] !== '');

fixes #695

Warning : Not ready for merge. The changes have been reverted to showcase the issue with existing implementation.

frederikbosch commented 2 years ago

Can you include a test on the example that fails?

sudkumar commented 2 years ago

@frederikbosch I have reverted the implementation and added a test to show the failure.

frederikbosch commented 2 years ago

@sudkumar The tests pass. So the question is: why does it fail on your end?

sudkumar commented 2 years ago

Hmmm.. interesting 🤔. BTW, there are two directories of tests. composer test executes from specs dir.

frederikbosch commented 2 years ago

What is inside $matches when you execute the test? Can you show the result of the following var_dump here?

var_dump($iso, $matches);
sudkumar commented 2 years ago

Here it is.

> var_dump($iso, $matches);

string(16) "EUR/USD 1.250000"
array(4) {
  [0]=>
  string(16) "EUR/USD 1.250000"
  [1]=>
  string(3) "EUR"
  [2]=>
  string(3) "USD"
  [3]=>
  string(8) "1.250000"
}

System:

Here is the exception when running composer test

----  broken examples

        Money/Currencies/BitcoinCurrencies
  32  ! is iterable
        exception [err:AssertionError("assert(is_array($subject))")] has been thrown.

        Money/Currencies/CurrencyList
  45  ! is iterable
        exception [err:AssertionError("assert(is_array($subject))")] has been thrown.

        Money/CurrencyPair
  30  ! parses an iso string
        exception [err:AssertionError("assert(is_numeric($matches[2]))")] has been thrown.

21 specs
108 examples (105 passed, 3 broken)
150ms
sudkumar commented 2 years ago

What do you get with var_dump($iso, $matches); ?

sudkumar commented 2 years ago

CleanShot 2022-08-10 at 20 12 09

Am I missing something obvious here!! 😄

frederikbosch commented 2 years ago

@sudkumar Thank you very much, I have fixed the issue. zend.assertions was not enabled in CI. That's why it was not throwing. Your observation was correct.

sudkumar commented 2 years ago

So glad we found the issue. Thank you for looking into it.