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

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

Closed
wants to merge 6 commits into from

Conversation

olekhy
Copy link
Contributor

@olekhy olekhy commented May 18, 2015

  • shouldn't use stringtype

@danizord
Copy link
Member

@olekhy why? Currency actually implements __toString().

@prolic
Copy link
Contributor

prolic commented May 19, 2015

I definitly expect a currency object returned and not a string. Second argument for Money constructor is a Currency instance, not any string is allowed. A currency type returning a string (that is not a currency) is pointless.

@olekhy
Copy link
Contributor Author

olekhy commented May 19, 2015

@danizord
because it's make side effekt for.
as f.e.

\Money\Money::__construct($amount, Currency $currency) build a money with a currency instance

after on \Money\Money::getCurrency returned a string

    /**
     * @return \Money\Currency
     */
    public function getCurrency()
    {              var_dump(gettype($this->currency));
        return $this->currency;
    }

when calling in:

class MoneyHydrator implements HydratorInterface
{
    /**
     * {@inheritDoc}
     */
    public function extract($object)
    {                 var_dump(gettype($object->getCurrency())); die;
        return [
            'amount' => $object->getAmount(),
            'currency' => $object->getCurrency()->getName()
        ];
    }

output:

string 'string' (length=6)

string 'string' (length=6)

@olekhy
Copy link
Contributor Author

olekhy commented May 20, 2015

👏 wakeup here we needed a worked module... or w'll use my branch for composer installation. ;) temporary.

olekhy and others added 3 commits May 20, 2015 13:46
 * at mongo persistence should nor a instance only string
@fabiocarneiro
Copy link
Member

@olekhy Sorry for the delay. Ill review that ASAP.

@olekhy
Copy link
Contributor Author

olekhy commented May 23, 2015

@fabiocarneiro Thank you, we will gladly wait for.

@@ -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
Copy link
Member

As we already discussed in #22. There is no reason to the conversion to be done in the Hydrator and it would also break BC.

If you want to properly display it in you form and it's not working, you should do a Form View Helper instead.

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)

@olekhy
Copy link
Contributor Author

olekhy commented May 27, 2015

@danizord @fabiocarneiro i'll close my pr's and hope u implements:

  • form element
  • CurrencyType included expected improvement.

ok so?

@danizord danizord added this to the 0.2.1 milestone Jun 4, 2015
This was referenced Jun 4, 2015
@danizord
Copy link
Member

danizord commented Jun 4, 2015

@olekhy Ok, I'll take the Form element (opened #27).

But I'm still not sure about the mongo type (I know nothing about MongoODM). Can you send a separate PR for this one?

@danizord danizord closed this Jun 4, 2015
@olekhy
Copy link
Contributor Author

olekhy commented Jun 4, 2015

@danizord yeah. Currency type I'll make a separate PR.

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

Successfully merging this pull request may close these issues.

4 participants