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.

Sign up for GitHub

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

MoneyHydrator fixed #22

Closed
wants to merge 2 commits into from
+2 −2
Closed

MoneyHydrator fixed #22

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57c6d31
MoneyHydrator fixed
olekhy May 18, 2015
1e9774e
MoneyHydrator fixed unit test
olekhy May 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Hydrator/MoneyHydrator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class MoneyHydrator implements HydratorInterface
public function extract($object)
{
return [
'amount' => $object->getAmount(),
'amount' => $object->getAmount() / 100,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is reponsability of hydrator. What do you think about having a custom form element for amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for diving per 100 an extra custom form element is way to much, imho.

Copy link
Member

Choose a reason for hiding this comment

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

There are some currencies that have more than 100 units like INR, so dividing by 100 would result in incorrect values.

Converting to the desired unit is a "view layer" issue. If you're using Zend\Form, that would be your view layer. If you're using API's or SPA, its recommended to return the units, so you convert it in javascript or in the client.

If you get the extracted array and place it into hydrate method, it should return the exactly the same object.

Copy link
Contributor

Choose a reason for hiding this comment

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

First:
I don't know about all currencies, but INR (indian rupee) is definitly not more than 100 units.
The modern rupee is subdivided into 100 paise (singular paisa), see http://en.wikipedia.org/wiki/Indian_rupee.

Second:
Considering dividing 100 or not (with the current module, as it is):
a) You render a form and enter 100 as value and you select USD as currency.
b) money get hydrated and a value of 100 is stored into its money amount property.
c) again 100 is saved to database.
d) last: let's print the value using the given moneyFormat view helper: 1,00 USD ! WTF? I entered 100 USD in a form and get only cents rendered.

Last point: If there is really some currencies out there with more that 100 fractions, it's better to add a getPrecision() method to the currency object itself instead of juggling with divining of 100 in various places.

Best would be, if the module takes care of everything and I don't need to deal with such silly details.

Copy link
Member

Choose a reason for hiding this comment

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

If you want 100 USD, that's 10000 units. You should never persist 100. You should set 10000 in the form. It wil save 10000 in the database and when you return it, you'll have 10000 in money object.

When you return the money object. The MoneyFormat helper already knows about that and does exactly what you suggested, which is dividing it by 100. But that is a "temporary" solution, and its expected from the helper to work with non decimal currencies, since it's in the view layer and this is view layer responsability. http://en.wikipedia.org/wiki/Non-decimal_currency

'currency' => $object->getCurrency()->getName()
];
}
Expand Down