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

Money's internal amount should be a string #134

Closed

Conversation

frederikbosch
Copy link
Member

Fixes #133.

@frederikbosch
Copy link
Member Author

In order to create a MoneyParser PR, I need this to be merged. I just improved more docblock and squashed my commit.

*
* @return int
*/
private function castString($amount)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need a separate method for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we really do not need it. Removed it, and squashed again.

@@ -47,7 +47,7 @@
];

/**
* @param int $amount Amount, expressed in the smallest units of $currency (eg cents)
* @param int|string $amount Amount,expressed in the smallest units of $currency (eg cents)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say we need that space after the comma.

@sagikazarmark
Copy link
Collaborator

Well, I tend accept this merge, but still need to get used to the fact that amount will be stored as string.

@frederikbosch
Copy link
Member Author

@sagikazarmark I agree. It is a big change. #115 was more comprehensive than I thought at first.

@sagikazarmark
Copy link
Collaborator

Merging the deprecation PR caused some conflicts.

@frederikbosch
Copy link
Member Author

Yeah, and now I need to rebase? :) Still don't know how, sorry about that...

@sagikazarmark
Copy link
Collaborator

No problem, I will do it.

@sagikazarmark
Copy link
Collaborator

Conflicts solved in #136

@josecelano
Copy link

Some months ago I was testing how to deal with bit integers and I did this fork:

https://github.com/josecelano/money/blob/master/lib/Money/BigMoney.php

Instead of using integer or string for the amount I used another class. In this implementation Money has not the responsibility to deal with bit integers and it delegates to another class. The only problem is that library uses only bcmath extension. But maybe there is another similar class which uses calculatorsas you did it.

@frederikbosch
Copy link
Member Author

@josecelano bcmath is the only option for PreciseMoney. I rather use that term than BigMoney, because we already have BigMoney because there is no more integer limit. GMP only uses integers, not decimals, see the RFC for floating points integration in PHP.

The solution I will propose soon will have the following structure.

  • PreciseCalculator with methods like roundPrecisely
  • PreciseMoney
  • BcMathCalculator implementing PreciseCalculator
  • I still have to think have to deal with formatting and parsing.

When the RFC for GMP lands, we only have to implement PreciseCalculator for GMP.

@josecelano
Copy link

Sorry,there was definitely a misunderstanding. What I called BigMoney is an implementation of Money which allows big integers, not decimals. I used Decimal class but internally I force the constrain of not using decimals, I mean all values would have no deciaml part even if that class allows it. But the idea I want to expose is you could use another class to store the amount and delegate to that class the responsabiliti of dealing with bit integers. In fact, I was looking for a BigInteger class but I do nto remember why finally I decided to use Decimal for that test.

@sagikazarmark
Copy link
Collaborator

Yeah, BigMoney casued a lot of confusion in me as well. But it refers to the precision of the value, not the integer size.

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.

3 participants