Fix number comparison in number_to_amount() in number_helper.php

[sba]

Description
1000 gives 1 thousand and not 1000
1000000 gives 1 million and not 1000 thousand
and so on...

And it compares now the same way like the function number_to_size above

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

This seems reasonable.

Can you add test code and changelog?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.3.7.rst#changes
Because this clearly changes the behavior of the function.

[sba]

Can you add test code and changelog?

Yes, I will do this in the next days.

What do you think about the command line 81 $num = 0 + str_replace(',', '', $num); above?

This command is dangerous in my opinion.
In German and other countries, the comma is used as a decimal separator. Decimal_separator
number_to_amount('1000000,00') should output 1 million but it outputs now 100 million.

And all other chars like apostroph or whitespace used as thousends separator are not stripped anyway.
number_to_amount("1'000'000"); returns false instead of 1 million, apostroph is used as thousends separator in Switzerland.
number_to_amount("1 000 000"); returns false instead of 1 million, a space or a dot is used as thousends separator in German.

[kenjis]

The function has the parameter $locale. So it is better to behave according to the locale.
If you need to fix the behavior in German or other locales, please send another PR to fix.

[sba]

to make behaviour dependent on language makes it too complex for this simple function.
I would only allow a number/int as a input-parameter. Or return the unformatted string if the input can not be casted to int.
But let's leave it as it is, such a change would change the behaviour of the function a lot...

[kenjis]

We have a plan to release next bug fix (and 4.4.0) soon.
Can you add a test and docs?

[sba]

While writing the tests I came across further counterintuitive behaviour.
Check following code:

helper('number');
$testnumbers = [
    1000
    , 999_499
    , 999_500
    , 999_999
    , 1_000_000
    , 1_499_999
    , 1_500_000
];
foreach ($testnumbers as $number) {
    $result[$number] = number_to_amount($number);
}
print_r($result);

Output:

Array
(
     [999499] => 999 thousand
     [999500] => 1,000 thousand
     [999999] => 1,000 thousand
    [1000000] => 1 million
    [1499999] => 1 million
    [1500000] => 2 million
)

In my opinion, the correct output for the number 999500 would be 1 million and not 1,000 thousand.
But the function cannot be adapted so easily because the parameter precision would have to be taken into account...
What do you think?

[kenjis]

If you want to fix it, something like this (to all cases)?

--- a/system/Helpers/number_helper.php
+++ b/system/Helpers/number_helper.php
@@ -106,6 +106,11 @@ if (! function_exists('number_to_amount')) {
         } elseif ($num >= 1000) {
             $suffix = lang('Number.thousand', [], $generalLocale);
             $num    = round(($num / 1000), $precision);
+
+            if ($num >= 1000) {
+                $num = 1;
+                $suffix = lang('Number.million', [], $generalLocale);
+            }
         }
 
         return format_number($num, $precision, $locale, ['after' => $suffix]);

[kenjis]

@sba Thank you for update!

This PR contains a merge commit that is not needed.
Can you git rebase to remove the merge commit?
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

[sba]

@kenjis sorry, I tried, but I don't get it with git rebase... i do it a bit primitive in the online editor on github.com to have verified commits. I will do a new PR with the changes.

[kenjis]

@sba Thank you for trying.

I hope you learn to use git rebase, because if you can, you don't need to create a new PR.
You can modify any commits in the PR branch.

If you need help, feel free to ask.