pronamic / wp-money

WordPress Money library.
https://pronamic.github.io/wp-money/
5 stars 1 forks source link

Logic `Money->format( $format = null )` and `Money->format_i18n( $format = null )` confusing? #2

Open remcotolsma opened 3 years ago

remcotolsma commented 3 years ago

In both functions the $format parameter is optional, but the parameter doesn't work the same way. If null is passed in Money->format_i18n( $format = null ) function it will fall back to Money::get_default_format():

https://github.com/pronamic/wp-money/blob/86b41ad9de8ab482be6adbeace317136177fdbcf/src/Money.php#L71-L73

If null is passed in Money->format( $format = null ) function it will fall back to '%2$s':

https://github.com/pronamic/wp-money/blob/86b41ad9de8ab482be6adbeace317136177fdbcf/src/Money.php#L127-L129

Currently it's very imported that Money->format( $format = null ) falls back to '%2$s', otherwise some of the gateways will break:

https://github.com/wp-pay-gateways/mollie/commit/08bc30cbf727a1820f38f99fa76a5bd7cfdf56c1

However, it is not very logical that the Money->format( $format = null ) does not process currency information by default, without currency information it's just a number.

We could make this more clear like this: Money->format_amount( $number_decimals = null );, if $number_decimals is null we can fall back to the currency $number_decimals value, similarly an i18n version: $money->format_i18n_amount( $number_decimals );.

Naming can also be different:

I think i prefer number_format and number_format_i18n:

The default format for Money->format( $format = null ) can then be changed to '%3$s%2$s' so that it contains at least the currency code by default.

Maybe we should also consider replacing the printf directives with placeholders:

$placeholders = array(
    '{non_breaking_space}'            => ...,
    '{currency_symbol}'               => ...,
    '{currency_alphabetic_code}'      => ...,
    '{currency_numeric_code}'         => ...,
    '{currency_name}'                 => ...,
    '{amount}'                        => ...,
    '{amount_i18n}'                   => ...,
    '{amount_no_trailing_zeros}'      => ...,
    '{amount_i18n_no_trailing_zeros}' => ...,
);

Current use is not very clear:

echo $money->format( '%3$s%2$s' );

You have no idea what the result is, following is more clear:

echo $money->format( '{currency_alphabetic_code}{non_breaking_space}{amount}' );

@rvdsteege thoughts?

rvdsteege commented 3 years ago

New methods added by @remcotolsma in https://github.com/pronamic/wp-money/commit/a08b3f8d3a4a0434db06d3dea349c9a31c29b3d2: