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

Bug fixation of ODM\MongoDB\Types\CurrencyType #21

Closed
wants to merge 6 commits into from
Closed

Bug fixation of ODM\MongoDB\Types\CurrencyType #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ae2098
Bug fixation of ODM\MongoDB\Types\CurrencyType
olekhy May 18, 2015
57c6d31
MoneyHydrator fixed
olekhy May 18, 2015
1e9774e
MoneyHydrator fixed unit test
olekhy May 18, 2015
659e434
Merge pull request #2 from olekhy/fixation-bug-CurrencyType
olekhy May 20, 2015
4330d3e
Merge pull request #1 from olekhy/fixed-MoneyHydrator
olekhy May 20, 2015
51aeb14
Fixation bug currency type
olekhy May 21, 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.

The purpose of mathiasverraes/money is to always store and work with money represented in the smallest unit (because of rouding erros).

Yeah, of course we don't want to display it in cents for our user, that's why the $amount / 100 is a presentation concern, so that you must handle it in view layer. I think a custom form element does the trick.

Thoughts? @olekhy @prolic @fabiocarneiro @gabrielsch

Copy link
Contributor

Choose a reason for hiding this comment

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

A user will always enter "1.25 USD". We have to have 125 as amount value. So putting this behaviour in a custom form element, which translates (in zf2 terms: filters) the input (1.25) to 125. The hydrator then does not need to divide by 100. Good catch @danizord !

About the closureToPhp and closureToMongo methods: They are indeed needed for doctrine odm mongodb adapter to work. They do not exist in doctrine orm.
We tested the current mongo implementation (which I contributed to, too), but the automatic string casting does not happen. Additionally we have to concider the null checks. So this in needed in order to work cleanly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prolic @danizord

  • ok for testing evangelista, null could be tested for.
  • form elem how implements this one i hope brazil. (pls using of attribute "step" should be skipped on config maybe)

'currency' => $object->getCurrency()->getName()
];
}
Expand Down
18 changes: 16 additions & 2 deletions src/ODM/MongoDB/Types/CurrencyType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@

namespace ZFBrasil\DoctrineMoneyModule\ODM\MongoDB\Types;

use Doctrine\ODM\MongoDB\Types\StringType;
use Doctrine\ODM\MongoDB\Types\Type;
use Money\Currency;

class CurrencyType extends StringType
class CurrencyType extends Type
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this one? We still can inherit convertToDatabaseValue() and closureToMongo() from StringType since Currency implements __toString() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry no, this is an falsely behavior Currency isn't a string type, BTW doctrine2 and proxy manager (we used) doesn't automatically cast to string. Not useabale with u proposed __toString

Copy link
Member

Choose a reason for hiding this comment

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

Well, in DB side it is a string, so I think it's ok to inherit VO -> string conversions from StringType. Just a suggestion, though. Result still the same :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

almost all data mapped to string on database side, is more interesting PHP side. because we speak from ODM (ORM) here. Look at times http://doctrine-dbal.readthedocs.org/en/lates/reference/types.html#custom-mapping-types for u should be a float type :). (may better way ObjectType -> serializing of money inclusive currency as one field)

{
const NAME = 'currency';

Expand All @@ -14,6 +14,15 @@ public function getName()
return self::NAME;
}

public function convertToDatabaseValue($value)
{
if ($value) {
return (string) $value;
}

return null;
}

public function convertToPHPValue($value)
{
if ($value === null || $value instanceof Currency) {
Expand All @@ -22,4 +31,9 @@ public function convertToPHPValue($value)

return new Currency($value);
}

public function closureToPHP()
Copy link
Member

Choose a reason for hiding this comment

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

@olekhy Can you explain the purpose of this method? I'm not familiarized with this conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiocarneiro http://doctrine-mongodb-odm.readthedocs.org/en/latest/reference/basic-mapping.html#custom-mapping-types

{
return '$return = new \Money\Currency($value);';
Copy link
Member

Choose a reason for hiding this comment

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

You should probably consider cases when $value is null:

return '$value !== null ? new \Money\Currency($value) : null';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is never null because money w/o currency is a nonsense! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be null! Currency can be used outside of money context and could be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree in this abstraction

}
}
2 changes: 1 addition & 1 deletion test/Hydrator/MoneyHydratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public function testHydratorExtractAsExpected()
$object = new Money(500, new Currency('BRL'));
$hydrator = new MoneyHydrator();
$extracted = $hydrator->extract($object);
$expected = ['amount' => $object->getAmount(), 'currency' => $object->getCurrency()->getName()];
$expected = ['amount' => 5, 'currency' => $object->getCurrency()->getName()];

$this->assertEquals($expected, $extracted);
}
Expand Down