parse a string into a money object

[frederikbosch]

Fixes #98 and provides a possible solution for #97.

[frederikbosch]

Hmm, why is this failing on Travis while tests are succeeding on my machine?

[barryvdh]

Could it be that the locale is not available on the Travis setup?

This gives the same (unclear) message: https://3v4l.org/1p9eF

[sagikazarmark]

Travis only has en_US installed. The funny thing is that I didn't have this issue with/without nl_NL installed, I had totally different issues.

I would rewrite the test examples to use en_US locale, it is probably commonly installed for others anyway. I would also skip tests on machines where en_US locale is not available.

[frederikbosch]

@sagikazarmark Right, we should do that. But I doubt if people will have same results when using the classes. That might lead to too many unexpected results.

[sagikazarmark]

That might lead to too many unexpected results.

What might lead to ...?

I think I missed an important part: we should try to set the locale to en_US at the beginning of the test. If it fails, skip.

[frederikbosch]

@sagikazarmark I agree with that. But I do not get why your results with nl_NL are so different. That tells me that we have an unstable API.

[sagikazarmark]

I didn't pay too much attention, maybe something else was wrong. It is PHP after all, stability is not one of the keywords. Also, intl is especially something vague.

[barryvdh]

Hmm, I don't have a lot of experience here, but if results are unreliable, I'm not sure I'd trust this to handly my money stuff.

[sagikazarmark]

I think we could preserve the "original" parsing logic in a separate parser, which was less smart, but more stable.

[frederikbosch]

@barryvdh Do not get this wrong: Money is reliable. We are talking about an unmerged PR on parsing and formatting negative values with intl extension.

@sagikazarmark We should look into this more before drawing conclusions.

[sagikazarmark]

Agree, I didn't want to give up on intl extension, but for simpler cases, the previous logic could be useful as well, like SimpleParser.

[frederikbosch]

@sagikazarmark The PR already contains the original parser.

[sagikazarmark]

👍

[barryvdh]

Yes I understand Money is stable, I was just referring to intl parsing by itself ;)

[sagikazarmark]

Okay, but I guess this way you are able to stick to the "default/previous" strategy or come up with a better, since we have an interface for that and no pressure on which to use.

[frederikbosch]

@sagikazarmark Why don't we inject NumberFormatter into IntlMoneyParser and IntlMoneyFormatter?

This we place behaviour of the intl extension outside of Money. As such we only have to guarantee/test the conversion, and not the results of NumberFormatter itself like we are doing now.

[sagikazarmark]

I remember I had some issues with NumberFormatter itself, and this injection approach, but I can't see any better solution. Although, it would be nice to look around and find some usage examples.

My biggest concern is the required configuration in of the NumberFormatter. It needs some basic config to work well (which is not necessarily configurable later).

[frederikbosch]

Yes, if Money provides an intl parser and formatter, at least we should know how to use it! Hence, we should know how to configure it.

But, now the intl parser and formatter implementations are instantiating the NumberFormatter object. This limits the user in configuring the NumberFormatter how he or she wants it. For example, the current class does not allow a developer to create a pattern.

[frederikbosch]

Okay. When constructing an NumberFormatter as below, the outcomes are different because the Travis is using a different default pattern.

$formatter = new \NumberFormatter('en_US', \NumberFormatter::CURRENCY);
$config = [ .. ];
var_dump($config);

Travis:

array(9) {
  'locale_valid' =>
  string(5) "en_US"
  'locale_actual' =>
  string(2) "en"
  'pattern' =>
  string(23) "¤#,##0.00;(¤#,##0.00)"
  'attr_positive_prefix' =>
  string(1) "$"
  'attr_positive_suffix' =>
  string(0) ""
  'attr_negative_prefix' =>
  string(2) "($"
  'attr_negative_suffix' =>
  string(1) ")"
  'attr_padding_char' =>
  string(1) "*"
  'attr_currency_code' =>
  string(3) "USD"
}

My Ubuntu (14.04) machine:

array(9) {
  'locale_valid' =>
  string(5) "en_US"
  'locale_actual' =>
  string(2) "en"
  'pattern' =>
  string(10) "¤#,##0.00"
  'attr_positive_prefix' =>
  string(1) "$"
  'attr_positive_suffix' =>
  string(0) ""
  'attr_negative_prefix' =>
  string(2) "-$"
  'attr_negative_suffix' =>
  string(0) ""
  'attr_padding_char' =>
  string(1) "*"
  'attr_currency_code' =>
  string(3) "USD"
}

What is causing the default pattern to be different?

[frederikbosch]

So, now I got a green light.

$formatter = new \NumberFormatter('en_US', \NumberFormatter::CURRENCY);
$formatter->setPattern("¤#,##0.00;-¤#,##0.00");

$parser = new IntlMoneyParser($formatter);
$this->assertEquals($units, $parser->parse($string, 'USD')->getAmount());

Only setting a string with double quotes which include a pattern for positive and negative amounts via the setter (not via constructor) will give a stable result.

[frederikbosch]

@sagikazarmark So we now have two parsers.

• The easy regex based one, called StringToUnitsParser. That one only parses a string into an amount. The developer is required to supply the currency.
• The intl one for the developer's advanced locale based parsing. This parser also parses a currency symbol, but can behave differently per system if not setting a pattern explicitly.