WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 7 years ago

#10555 closed defect (bug) (fixed)

number_format_i18n function doesn't check if the number is float or not before doing the formating

Reported by: chombium Owned by: nbachiyski
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: I18N Keywords: has-patch dev-feedback
Focuses: Cc:
PR Number:

Description

Hi,

Problem description:

I've noticed that there are problems in representation of decimal numbers. The number representation is defined by the following three values in the wordpress.pot translation file: number_format_decimals, number_format_decimal_point and number_format_thousands_sep.

In the Macedonian language translation of wordpress.pot which I
maintain, I've set this values to:
number_format_decimals = 3
number_format_decimal_point = ,
number_format_thousands_sep = .

This means that the proper decimal number representation would be 1.234,567

I've noticed that all the numbers including integers are shown with this format, so for example, I get "23,000" posts on the dashboard instead of "23".

I can fix this behavior by setting:
number_format_decimals = 0
in the translation, but if a floating point number has to be formated it will be truncated to its integer part.

This way all the floating point numbers are not formated correctly, but as I was told by Nikolay Bachiyski, this is the safe way to solve this issue because this function is called only with integers.

I dig trough the code of wp 2.8.3 and I've noticed that the problematic function is number_format_i18n located in wp-includes/functions.php
This function is the only place where php number_format function is
called with the parameters from the translation file.

Solution:

The function number_format_i18n has to be patched so that first it checks what kind of number should be formated (integer or float) and then to do the formating. This means it has to set the correct number of decimal places before calling the php number_format function.

This can be done by adding the line:

if (!is_float($number)) $decimals = 0;

after setting the number of decimals, the line:

$decimals = ( is_null( $decimals ) ) ? $wp_locale->number_format['decimals'] : intval( $decimals );

The proposed patch simply checks if the number which has to be formated is not float and if that is the case it sets the number of decimals to 0 ($decimals = 0)

I don't know if the file: wp-content/plugins/akismet/akismet.php has to be patched as well because it redeclares the number_format_i18n function if it doesn't exist. I see that the function is called with one parameter in this file, which means decimals = 0.

The diff file of wp-includes/functions.php is in the attachment.

Jovan

Attachments (4)

functions.php.diff (154 bytes) - added by chombium 10 years ago.
functions.php diff file
10555_float_i18n.diff (701 bytes) - added by nacin 10 years ago.
10555.diif (917 bytes) - added by westi 10 years ago.
Alternative patch which preserves the functionality if the callee supplies a precision
number-format-int-only.diff (5.1 KB) - added by nbachiyski 10 years ago.

Download all attachments as: .zip

Change History (25)

@chombium
10 years ago

functions.php diff file

#1 @scribu
10 years ago

  • Keywords number formating i18n removed
  • Milestone changed from Unassigned to 2.9

#2 @nacin
10 years ago

  • Keywords needs-testing added
  • Version changed from 2.8.3 to 2.9

Proper/refreshed patch. Could use a test.

#3 @westi
10 years ago

  • Cc westi added
  • Keywords early added
  • Milestone changed from 2.9 to 3.0

Ok.

So the issue here is that if you specify the number of significant figures to show for a particular locale then that is used regardless of whether or not the number is a float.

I think at present everything we format is an integer hence the recommendation to stick with 0 for the value in the translation file.

I have an alternative patch for this but I think the best solution for you for 2.9 is to stick with 0 in the translation file like the other translators do and we can look at what to change here early in 3.0 - I wonder whether or not this value should be offered up for translation at all as it is doesn't feel like a locale issue how many significant figures are displayed

@westi
10 years ago

Alternative patch which preserves the functionality if the callee supplies a precision

#4 @chombium
10 years ago

The problem that I experienced was because there was no documentation that only integers are passed to this function and that there is no remark that the value for the decimals in the translation file should always be 0.

Because the value is already in the translation file and if the function is patched, I think the best way to solve this issue is to add a remark in the translation file and to update the http://codex.wordpress.org/Translating_WordPress page, that for now, the msgid "number_format_decimals" should be set to 0.

The comment:
"#. translators: $decimals argument for http://php.net/number_format, default is 0"
in the file http://svn.automattic.com/wordpress-i18n/pot/trunk/wordpress.pot should be updated with something like "set it to 0" as well

#5 follow-up: @hakre
10 years ago

I'm a bit confused. We have a pretty well report here, we have a parameter here "number_format_decimals" which actually is configured to values other than 0.

So if there is a parameter then it can get values other than zero and that function should refelct it.

If infact that parameter is not needed because we are only handling integers within that function, then this parameter is just overhead and should be removed.

If you're fighting with the reality that both is the case, why don't you just extend the function that it can deal with any case so everybody is happy?

#6 in reply to: ↑ 5 ; follow-up: @westi
10 years ago

Replying to hakre:

I'm a bit confused. We have a pretty well report here, we have a parameter here "number_format_decimals" which actually is configured to values other than 0.

The point is that I think this was made translatable by accident.

I don't believe that the number of digits displayed after a floating point it something that a translation team would be expected to specify or need to specify.

They need to specify the thousands separator and the decimal point.

We have a translation recommendation to always set this to 0 because it will display numbers as floats.

I am going to take this discussion to the wp-polyglots list to see if there is a need there for this to remain translatable.

#7 in reply to: ↑ 6 @hakre
10 years ago

Replying to westi:

Replying to hakre:

I'm a bit confused. We have a pretty well report here, we have a parameter here "number_format_decimals" which actually is configured to values other than 0.

The point is that I think this was made translatable by accident.

Makes sense.

I am going to take this discussion to the wp-polyglots list to see if there is a need there for this to remain translatable.

Just update this ticket when you have feedback.


If anyone can reference, please do name a language / culture where it is common to have 3 places after the floating point. Can someone please confirm this for the Macedonian language as suggested by the reporter? Please add hyperlinks to that info.

#8 @nbachiyski
10 years ago

Everywhere in WordPress number_format_i18n is used for integers only. The simplest solution is to make it work only with integers and deprecate all the decimals stuff. If at any point we need the floating point behaviour, we can always put the functionality back.

My patch deprecates the decimal arguments for number_format_i18n and size_format and locale-specific decimals and decimal point settings.

#9 follow-up: @scribu
10 years ago

Since we're on the topic, wouldn't it be nice to rename the function to number_i18n, to be consistent with date_i18n ?

#10 in reply to: ↑ 9 ; follow-up: @nbachiyski
10 years ago

Replying to scribu:

Since we're on the topic, wouldn't it be nice to rename the function to number_i18n, to be consistent with date_i18n ?

It is consistent. The PHP functions we are replicating are number_format() and date().

#11 in reply to: ↑ 10 @scribu
10 years ago

Replying to nbachiyski:

Replying to scribu:

Since we're on the topic, wouldn't it be nice to rename the function to number_i18n, to be consistent with date_i18n ?

It is consistent. The PHP functions we are replicating are number_format() and date().

Then again, PHP isn't know for it's consistent function names...

Either way, I like typing less. :-)

#12 follow-up: @nbachiyski
9 years ago

  • Keywords dev-feedback added; needs-testing early removed
  • Status changed from new to accepted

Any objections to the patch?

#13 in reply to: ↑ 12 @westi
9 years ago

Replying to nbachiyski:

Any objections to the patch?

Looks good to me.

Giving it a little love to fix the rejects and it will be in soon :-)

#14 @westi
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [14184]) Remove unnecessary translations of decimal point character and number of decimal places. Fixes #10555 props nbachiyski.

#15 follow-up: @brentes
9 years ago

I just updated to the latest and saw all the decimals in my plugin disappear.

I understand everything in WP uses integers, but please consider that many plugins, including all that deal with currency, use floats.

#16 in reply to: ↑ 15 @westi
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to brentes:

I just updated to the latest and saw all the decimals in my plugin disappear.

I understand everything in WP uses integers, but please consider that many plugins, including all that deal with currency, use floats.

We could restore this functionality by reverting part of this patch.

If we bring back the decimal seperator translation and just expect the callee to define the number of decimal places.

Would this be a suitable solution

#17 @westi
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [14190]) Restore support for floating point numbers in number_format_i18n(). Fixes #10555.

#18 @westi
9 years ago

Try that on for size :-)

#19 @brentes
9 years ago

Thanks Westi!

Needs a slight change though. Line 139 casts the number as an integer. So number_format_i18n( 1123.5813, 2) returns 1123.00, when you'd expect 1123.58.

#20 @nbachiyski
9 years ago

(In [14192]) Do not force formatted number to int. Props brentes, fixes #10555

#21 @lkraav
7 years ago

  • Cc lkraav added
Note: See TracTickets for help on using tickets.