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

FieldtypeDecimal needs to be in the core #126

Open
szabeszg opened this Issue Oct 25, 2017 · 19 comments

Comments

Projects
None yet
@szabeszg

szabeszg commented Oct 25, 2017

Well, what can I say? This is a must.

The issue is detailed here:
https://processwire.com/talk/topic/7542-development-fieldtypefloat-fieldtypedecimal/

Fork I happen to use:
https://processwire.com/talk/topic/7542-development-fieldtypefloat-fieldtypedecimal/?do=findComment&comment=139097

Text fields are not a solution but a workaround. Please consider solving it by adding FieldtypeDecimal to the core. It might need some clean up before it can be added but I think it is worth the effort.

It is such an important issue that it should not be left to third party development.

@teppokoivula

This comment has been minimized.

teppokoivula commented Oct 26, 2017

+1 for this request. While float is useful for many use cases and has better performance, it is not a good choice for storing exact (or large) numbers.

@arjenblokzijl

This comment has been minimized.

arjenblokzijl commented Oct 26, 2017

+1 for every calculation which needs precision we use decimals.

@kiwigringo

This comment has been minimized.

kiwigringo commented Dec 17, 2017

+1. This is a standard data type for almost every SQL dialect for reasons already described.
To make Processwire a viable contender for financial calculations particularly, this needs to be a core feature.

@BernhardBaumrock

This comment has been minimized.

BernhardBaumrock commented Dec 25, 2017

+1 :)

@x02a

This comment has been minimized.

x02a commented Jul 3, 2018

+1 ;)

@netcarver

This comment has been minimized.

netcarver commented Jul 4, 2018

Have to agree. +1

@jmartsch

This comment has been minimized.

jmartsch commented Sep 13, 2018

Please implement this.

@kongondo

This comment has been minimized.

kongondo commented Oct 7, 2018

+1 for the request, please. Will be useful for Padloper as well. Thanks.

@NikoTechnik

This comment has been minimized.

NikoTechnik commented Nov 23, 2018

+1 Please Implement!

@flydev-fr

This comment has been minimized.

flydev-fr commented Nov 29, 2018

I agree too. +1

float a = 0.15 + 0.15
float b = 0.1 + 0.2
if(a == b) // can be false!
if(a >= b) // can also be false!

Meanwhile...

What can I do to avoid this problem?

If you really need your results to add up exactly, especially when you work with money: use a special decimal datatype.
If you just don’t want to see all those extra decimal places: simply format your result rounded to a fixed number of decimal places when displaying it.
If you have no decimal datatype available, an alternative is to work with integers, e.g. do money calculations entirely in cents. 

The Floating Point Guide

@LostKobrakai

This comment has been minimized.

Collaborator

LostKobrakai commented Nov 29, 2018

While I'm all for having a nicely working decimal fieldtype in the ecosystem I've yet to see a reason why this needs to be in the core specifically. Decimals are important, but only to certain types of applications. Maybe someone can describe what's missing from the current module, which could only be fixed by adding it to the core or adding some capability to the core, which is missing by now.

@BernhardBaumrock

This comment has been minimized.

BernhardBaumrock commented Nov 29, 2018

Valid point. I wanted to comment that I think there should at least be a note or warning on the float fieldtype but then I went ahead and just created that as PR: processwire/processwire#130

img

I also created a tutorial of how to submit PRs so that maybe others can contribute easier and faster: https://processwire.com/talk/topic/20423-how-to-submit-pull-requests-to-the-processwire-core/

@flydev-fr

This comment has been minimized.

flydev-fr commented Nov 29, 2018

I don't think that a warning on the InputfieldFloat is needed here, but as a note in the documentation as I expect that a programmer know what is a Floating-point type, as it's the same story for others programming language.

I think it could might be worth writing an article on ProcessWire about, e.g., how to work with financial data, using decimal or integer type but not float and explaining performance issues doing calculations on a decimal type.

@szabeszg

This comment has been minimized.

szabeszg commented Nov 29, 2018

C'mon guys :) we are talking about ProcessWire which is supposed to ease PHP development. We might as well argue that there is no need for "jQuery style selectors" we only need to educate people about the PDO class. I could come up with hundreds of other similar examples, of course.

This is a popular request, it can be turned down by saying: just learn more about PHP but I do not think this is the way to go. Loads and loads of extra features have been added since this request was posted, features we could also live without. Why this is the questionable one?

@LostKobrakai

This comment has been minimized.

Collaborator

LostKobrakai commented Nov 29, 2018

It's certainly a valid request, but for all I know we have a working 3rd party solution. What would we gain from moving it into the core for the backdraws of ryan needing to become familiar with the fieldtype's inner working, ryan having one more fieldtype to maintain and the core getting bigger for everyone. That's what I asked about.

@NorHei

This comment has been minimized.

NorHei commented Dec 5, 2018

I think correct handling of numbers is a core concern in any application, floats implement tons of problems with precision and or small/big values.
I would even propose to replace the floats field in the core with this one so you don't have to expect surprises if you use the buildin field in your calculations.

@kiwigringo

This comment has been minimized.

kiwigringo commented Dec 5, 2018

The reason why this should be a core field type, is that it is an intrinsic data type in pretty much all SQL database systems.
Processwire core implements most other basic SQL data types in the core, even if some are abstracted.
(eg FieldtypeCheckbox abstracts mySQL's tinyint, which is in turn equivalent to bit)
From the perspective of someone considering web APIs and UIs for RAD development of data driven apps, it would be reasonable for them to expect Processwire to support all common SQL data types out of the box.
Although there is a perfectly functional third party module, anyone evaluating Processwire will not necessarily know about it without searching the module directory, and the omission of this key data type could result in a negative evaluation of Processwire.
If it's not included in the core, maybe another option might be to create a site profile that includes what might be considered essential third party modules.

@teppokoivula

This comment has been minimized.

teppokoivula commented Dec 6, 2018

Not sure if I have much to add to this discussion, but I'll try anyway.

First of all, I believe that it should be obvious at this point that the current, built-in float field is kind of limited in its scope: it can only handle relatively small numbers, and even then it may still cause issues behind the scenes by altering input values unexpectedly. Issues that could be difficult to observe and debug, I might add.

The obvious solutions would be to either implement a separate decimal fieldtype and explain why and when the user should use float / decimal, or perhaps entirely replace our current float type with a different implementation. The first solution would be preferable for experienced users who might want to choose the data type on a case-by-case basis (float for performance, decimal when precision or big numbers are needed), while the latter would be the most user friendly choice – I'd argue that a large number of ProcessWire users probably have no idea what the difference is, or which type to choose.

While I get that new features shouldn't be implemented on an impulse, I can see that there's plenty of support for this feature – and I believe that there would be more if folks only knew what we're dealing with here. The problem, again, is that most don't: while it might be true that "real programmers" know exactly how floating-point units work and how to handle financial data correctly, many of our users don't fit in that category.

Yes, there's a third party solution, but in my opinion this is one of those very basic things that should be handled properly in the core itself. Additionally it appears that this particular third party solution is in a bit of a flux: original maintainer doesn't seem to be active, so the recommendation is to use a fork – nothing we can't solve of course, but that's one more reason why important features like this should be in the core.

Finally, the points about Ryan having to "learn" what Decimal fieldtype is/does, or the core getting bigger, are – in my opinion – kind of moot. First of all I'd be surprised if he wasn't already familiar with decimal vs. floating point discussion, and second of all... look at this fieldtype. It's not massive by any scale, and it's really not particularly complicated :)

@teppokoivula

This comment has been minimized.

teppokoivula commented Dec 6, 2018

@ryancramerdesign, all things considered it'd be great if you could consider Decimal fieldtype for the core, and perhaps even for the upcoming major version – although I get that in that regard the schedule is rather tight.

On the plus side the third party solution could be used as a base, and since it has been out there for quite a while now, some user testing has definitely been done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment