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

[MoneyBundle] Configurable round mode #10204

Open
Roshyo opened this issue Mar 1, 2019 · 28 comments
Open

[MoneyBundle] Configurable round mode #10204

Roshyo opened this issue Mar 1, 2019 · 28 comments
Labels
Do not stale Important issues and PRs, that should not be stalled by Stale Bot Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). Future Issues and PRs which are blocked by outside constraints. RFC Discussions about potential changes or new features.

Comments

@Roshyo
Copy link
Contributor

Roshyo commented Mar 1, 2019

Describe the proposed solution
At the moment, Sylius is storing currencies in hundredth of currency. That's a really good point for all the reasons we all know. But it should be allowed to store currencies in thousandth or ten thousandth or whatever we want to.

To do so, at the moment we have to override some files like MoneyType, MoneyFormatter at least.

It should be nice if we just had to change a configuration parameter in yml to change this rounding so that we have a better configuration.

The main use case I see is for side projects that needs to store an exact amount that might itself be rounded from an other one. And the hundredth rounding mode might lose some informations at this point.

Describe alternatives you've considered
I don't see any other alternative than overriding previously given files.

Additional context

@mamazu
Copy link
Member

mamazu commented Mar 1, 2019

Well, I don't see a use-case for that. Is there a currency where one unit is split into thousands?

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 2, 2019

I have 2 in mind where I've been forced to change the rounding mode.

  1. Give a VAT including price to be stored without taxes and regiven with taxes (like 24.99€ with 20% of vat). If you store this in cents, it will be stored as 2083 cents (rounded from 2082,5 cents) then redisplayed as 25€. You lost this cent that commercially speaking might be necessary.
    If you store in thousandth instead of hundredth. This problem will not occur anymore.

  2. For an ERP project, we have to store prices of items/KG.
    But, the product might be sold per gram. to achieve this, we had to create a custom price formatter. Storing in cents was not an option at all. If the product is 1.5€/Kg, then in database, it's impossible to store the price in €/gram (0.150 €) and database only stores integers.
    In this case, we stored the price divided by 100 000

@mamazu
Copy link
Member

mamazu commented Mar 2, 2019

Okay, this makes sense. I mean feel free to make a pull request. And see what the Sylius people think.

@CoderMaggie CoderMaggie added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). RFC Discussions about potential changes or new features. labels Mar 4, 2019
@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 10, 2019

Coming back here with an update. Not entirely linked to this functionnality, but this might increase the following bug :

If we store a currency in ten thousands of a unit and this currency has crazy amounts, like Japanese Yen for example. If you sell an item at ¥499 777,27 (approx 4000 euros) or a cart total at this price. You'll reach the SQL limit.

I'm sure that at the moment it's not really a problem with the hundredth rounding mode and using currencies like euro, dollar or any currency that has the same value.

Any ideas ?
Changing the type in doctrine to bigint results in returning strings instead of int and that might be a lot of work to overcome this...

@mamazu
Copy link
Member

mamazu commented Mar 11, 2019

Valid point. For this we should use a different datatype but I'd suggest to use the BIGINT then.
Working with strings will make it very hard to handle currencies.

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 11, 2019

Doctrine does convert BIGINT to string in PHP

@mamazu
Copy link
Member

mamazu commented Mar 11, 2019

Never heard of this but looks like it. Then it doesn't matter.

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 11, 2019

As a matter of fact, it matters.
Changing integers to BIGINT will break PHP7.2 strict typing. All get/set will return/expect an int and will work with strings. So will calculators and so on...

@4c0n
Copy link
Contributor

4c0n commented Mar 13, 2019

Classic programming problem! So it seems that MySQL uses 32 bit signed integers (at least the max value of INT seems to match that). BIGINT can be bigger than the 64 bit max integer value in PHP.
It is common to use strings to solve that problem, however might not be very convienent as most users don't need those big numbers and you'll add a lot of complexity that way.

For some countries however the limit might be reached quite fast, just consider Vietnam for example:
https://www.xe.com/currencyconverter/convert/?Amount=50&From=EUR&To=VND

50 euro's is worth more than 1,000,000 dong.

In this case though the least significant bits are truly quite insignificant and people will ignore pretty much everything under a thousand, allowing them to divide by 1000 and use ecommerce platforms that can not really handle their currency (they typically place a k behind the price to indicate the kilo factor).

Not sure, but I don't think it is worth converting the system to work with strings, maybe there are other options?

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 13, 2019

I'm also quite against converting to strings. It might create huge problems...
But on the other hand, I don't see any other viable option...

@vvasiloi
Copy link
Contributor

It might be crazy, but what about splitting the price into 2 columns, one for the whole number and another for the decimal part? It might come as a feature flag and/or a plugin.

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 14, 2019

Like having a integerPriceand a decimalPrice but keeping the price in int in hundredth or thousandth or any other rounding mode but only for PHP ?
It would demand to explode the price in two parts when storing it in database (maybe via event listeners) but I think it wouldn't ask too much effort and nearly no refactor from an external POV of the class.

@4c0n
Copy link
Contributor

4c0n commented Mar 14, 2019

Thought about that as well, it would solve the problem for now (at least on 64 bit systems) it seems. Maybe a plugin would be better, because in most use cases you really don't need this. That way it's possible to try this out and see what the consequences are without being an integral part of the framework.

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 14, 2019

If it is in a plugin, I'd require it to be compatible with MoneyBundle used in standalone.

@vvasiloi
Copy link
Contributor

@Roshyo by having 2 fields I think there is no need to keep the price in hundredth or thousandth.
Currently in MySQL it's saved as a INT(11) signed, so it can store a price up to 8 digits, because it's multiplied by 100.
Splitting in 2 columns will allow storing prices up to 10 digits (max 2147483647 on 64 bit systems) regardless of the decimal part, because it will be stored separately.
Prices can also be stored as unsigned integers and the whole number can also be stored as BIGINT if possible.

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 14, 2019

@vvasiloi the original problem in this thread was : "Store currency in other rounding than hundredth" because of the above reasons.

Simply exploding price in 2 columns and remaining the simple "2 digits after the coma" does not solve the original issue. Or I did not understand what you said

@vvasiloi
Copy link
Contributor

@Roshyo if it will be stored in two columns, then there will be no need for rounding.
For example, on a 64 bit system, it will be possible to store a maximum price of 2147483647.2147483647.
But I see 2 problems which I'm not sure can be easy to overcome:

  • int typehints for prices
  • most if not all calculations related to prices are done with integers
    I was also thinking about a price class which will handle all the transformations and calculations.

@Roshyo
Copy link
Contributor Author

Roshyo commented Mar 14, 2019

I'm afraid I don't understand your point. the decimal part is an int, ok, but the value of 1 unit has to be defined. At the moment 1 unit in decimal is 1 cent (in € for example) but storing a cent and not a tenth of a cent will not solve the above problem. or the unit in your example for the decimal part is more one billionth ? Where 10.2147483647 = 10.21 € ?
But then we wouldn't be able to sell products at 10.50€ ?

@vvasiloi
Copy link
Contributor

Right, 10.50€ will be stored as 10 and 50 and the trailing zero will break the calculations.
Also, the leading zeroes will cause issues, ex: 10.05€.
Conclusion: storing decimal part as integer is a bad idea.

@pamil pamil added the Future Issues and PRs which are blocked by outside constraints. label Mar 19, 2019
@stale
Copy link

stale bot commented Jun 17, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Issues and PRs with no recent activity, about to be closed soon. label Jun 17, 2019
@stale stale bot closed this as completed Jun 24, 2019
@nbjohan
Copy link

nbjohan commented Jul 3, 2019

I am currently having an issue with roundig aswel.
In my case, having a 24% discount on every OrderItem.
Screenshot_20190703_124634

This makes the grand total inaccurate. A workaround could be applying the discount on the Order, but that causes issues with taxation.

@Roshyo
I'm not sure if this issue is the same as your initial issue, but I think this is an additional case for having more accurate storage of currency.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 3, 2019

Integrating https://github.com/moneyphp/money might be a good idea.

@CoderMaggie CoderMaggie added Do not stale Important issues and PRs, that should not be stalled by Stale Bot and removed Stale Issues and PRs with no recent activity, about to be closed soon. labels Aug 7, 2019
@CoderMaggie CoderMaggie reopened this Aug 7, 2019
@nbjohan
Copy link

nbjohan commented Nov 25, 2019

I am currently having an issue with roundig aswel.
In my case, having a 24% discount on every OrderItem.
Screenshot_20190703_124634

This makes the grand total inaccurate. A workaround could be applying the discount on the Order, but that causes issues with taxation.

@Roshyo
I'm not sure if this issue is the same as your initial issue, but I think this is an additional case for having more accurate storage of currency.

As a followup to my comment above:
The correct solution is to use a Distributor to calculate the discount each unit should receive.
The result can be applied automatically to the order (items/units) by using an Applicator.

This can be seen in the Sylius docs: https://docs.sylius.com/en/1.6/cookbook/promotions/custom-promotion-action.html#create-a-new-promotion-action

@zangarmarsh
Copy link

Sorry to ping to a stale issue, did you guys find out a solution for this issue? Is moneyphp really a viable solution?

@vvasiloi
Copy link
Contributor

vvasiloi commented Feb 4, 2022

@mbabker I saw this https://github.com/BabDev/MoneyBundle recently and I was wandering if you used it with Sylius?

@mbabker
Copy link
Contributor

mbabker commented Feb 4, 2022

Not with Sylius, no. If you're looking to move toward https://github.com/moneyphp/money then my bundle definitely helps with that integration, plus it provides a few extra features (i.e. serializing Money objects and validation constraints), but it'd be incompatible with Sylius' current pricing data model.

@vvasiloi
Copy link
Contributor

vvasiloi commented Feb 4, 2022

Thanks! I'm not yet actively looking to do that, but maybe one day...

@lemorragia
Copy link

I'm also having rounding problems, with euro currency. I've put an eCommerce solution online that sells small plastic parts (so almost every product has cents in its price). If I apply promotions, the rounding error kicks in when there are significant cents at play:

Example:
-one product, with unit price of 1.15
-one promotion, 10% discount using the sylius default order percentage discount

if I buy 20 items, the item total will be 23.00€ and the discount 2.3€ <- correct
if I buy 11 items, the item total will be 12.65€ (which is correct), but the discount calculated via getAdjustmentTotal will be 1.27.

When the error is present the order total is often correct nonetheless (the rounding error is too small to be taken into consideration), in other cases it will be wrong by some cents.

In the admin panel it's often wrong in the two columns displaying "discount" and "discounted price", as per output of "getDiscountedUnitPrice" and "getFullDiscountedUnitPrice" functions in OrderItem, which can lead to confusion when reviewing the order, because it makes it look like that discountedUnitPrice*quantity is different from the orderItemTotal

Screenshot_20230412_171407

Here, as an example with a 10% promotion, the discount is calculated as 0.13 even tough is should be precisely 0.12. The discounted unit price is then wrongly calculated as 1.07 but, weirdly, the subtotal "is correct", in the sense that it's calculated as 1.08*20 (almost...in fact it should be precisely 21.6)...it's not a big issue but indeed a scary one potentially

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not stale Important issues and PRs, that should not be stalled by Stale Bot Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). Future Issues and PRs which are blocked by outside constraints. RFC Discussions about potential changes or new features.
Projects
None yet
Development

No branches or pull requests

10 participants