Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix decimal column php type #1423

Closed
wants to merge 1 commit into from
Closed

Fix decimal column php type #1423

wants to merge 1 commit into from

Conversation

rmrevin
Copy link
Contributor

@rmrevin rmrevin commented Dec 4, 2013

No description provided.

@rmrevin
Copy link
Contributor Author

rmrevin commented Dec 4, 2013

Without this fix fractional part is lost when the model is saved.

@qiangxue
Copy link
Member

qiangxue commented Dec 4, 2013

Why will the fractional part be lost?

decimal is supposed to be a precise representation of a double value. If we turn it into a real double value, it will lose precision.

@rmrevin
Copy link
Contributor Author

rmrevin commented Dec 4, 2013

If there is such Russian locale for numbers (setlocale LC_NUMERIC), then as the decimal separator is a comma. In this case, when you save in mysql, the fractional part is lost. Should either stand alone English locale for numbers or replace the comma on the dot.

$i = 1.323;
setlocale(LC_NUMERIC, 'en_GB');
var_dump((double)$i); // float(1.323)

setlocale(LC_NUMERIC, 'ru_RU');
var_dump((double)$i); // float(1,323)

@qiangxue
Copy link
Member

qiangxue commented Dec 4, 2013

Since we are dealing with strings for decimals, I don't see such conversion could happen in the core framework code...

@rmrevin
Copy link
Contributor Author

rmrevin commented Dec 5, 2013

So the only solution.

str_replace(',', '.', (string)$value);

It's awful.

@cebe
Copy link
Member

cebe commented Dec 5, 2013

When dealing with formatting values in input fields you always have to do conversion. This is a general task that has to be done and there is nothing awful with that.
The php intl extension has formatters and parsers that can do this task for you:
http://www.php.net/manual/en/numberformatter.format.php - for displaying the value
http://www.php.net/manual/en/numberformatter.parse.php - parsing the input and get the value back.

@cebe cebe closed this Dec 5, 2013
@rmrevin
Copy link
Contributor Author

rmrevin commented Dec 5, 2013

This is a bad decision. Because before saving the values, I need to constantly check the current locale (site in several languages ​​with different date formats and money) and create an object NumberFormatter.

The framework should get rid of extra activities. I give the usual floating-point value. I do not have to think about what decimal separator used.

@cebe
Copy link
Member

cebe commented Dec 5, 2013

This could be seen as dupliacte of #504 then which needs a general solution.

@nineinchnick
Copy link
Contributor

I don't think it's a duplicate of #504 as this is about filtering (normalising) user input and not translating attribute values, which imho makes sense only for "dictionary" like attributes (ex. status labels).

For certain datatypes, like dates, currency or floats, I always include filtering in valudation rules to normalize the value and it depends on the context and locale.

@rmrevin
Copy link
Contributor Author

rmrevin commented Dec 5, 2013

What is the relationship between the variator and formatting numbers in different locales?

@cebe
Copy link
Member

cebe commented Dec 5, 2013

Sorry, I only had a quick look at #504 and someone mentionend formatting and parsing there so I missed that the initial issue was about something else.

For certain datatypes, like dates, currency or floats, I always include filtering in valudation rules to normalize the value and it depends on the context and locale.

this is how it should be done imo. If there is anything the framework could do in helping with this process, please post your ideas. You may open a new issue for it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants