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

ValueObject Money custom type #10335

Closed
Legion112 opened this issue Dec 21, 2022 · 7 comments
Closed

ValueObject Money custom type #10335

Legion112 opened this issue Dec 21, 2022 · 7 comments

Comments

@Legion112
Copy link

Legion112 commented Dec 21, 2022

The problem

  1. Use of Money Value object to store USD, BTC, etc

The money library stores the amount as a string for example 1.89 USD is stored inside class Money as {amount: "189", currency: "USD"}
Storing the amount in the database as a string would prevent sorting them by value, the decimal type is more appropriate for this task.
The problem is that I need to convert amount: "189" to the database type as 1.89 and back.
To be able to convert I need to know how many digits are after the decimal point for example bitcoin has 8 digits. This information could be retrieved from this method

1.89 BTC would need to be converted to 189000000 (Satoshi)
1.89 ETH would need to be converted to 189000000000000000000 (Wei)
The bakc conversion should be possible as well:
189000000 BTC(Satoshi) => 1.89 BTC
189000000000000000000 ETH(Wei) => 1.89 ETH

I see the problem above could be solved as follows:

  1. To allow DI for custom DBAL type.
  2. To pass all parameters DBAL type to be able to convert the value base on the currency.

From the doctrine side the following change will be needed:

  1. Remove final from \Doctrine\DBAL\Types\Type::__construct() to allow dependency injection.
  2. Add array $parameters = [] to convertToDatabaseValue and convertToPHPValue

Example of how it will be used.

https://github.com/Legion112/doctrine-embadable-share-column/blob/main/src/MoneyType.php and https://github.com/Legion112/doctrine-embadable-share-column/blob/main/src/Money.php

Why it is better to allow those manipulations on the ORM side?

In case of not allowed to do the conversion on the ORM side, we are limited to storing the amount in a database as a string. Which potentially removes the following features:

  1. You can't sort my amount as it is a string, not a decimal value.
  2. You can't do aggregation sum, min, max, etc.
@derrabus
Copy link
Member

Sorry, but I have no idea what you're asking for.

@Legion112
Copy link
Author

Legion112 commented Dec 22, 2022

I have updated the description of IDIA.
In a few words I need the following:

  1. Remove final from \Doctrine\DBAL\Types\Type::__construct() to allow dependency injection.
  2. Add array $parameters = [] to convertToDatabaseValue and convertToPHPValue

Do you see any negative effect of introducing the extension to ORM API?
I would create a merge request for that if it is fine to make such changes.

@Legion112 Legion112 changed the title Embadable which share column value between Custom Type which aware of related parameters Dec 22, 2022
@Legion112
Copy link
Author

Legion112 commented Dec 22, 2022

The change would need to be done in dbal and the ORM module...

@Legion112
Copy link
Author

Legion112 commented Dec 22, 2022

Hi, @Ocramius. I have found the following quote of yours:

DBAL types are not designed for Dependency Injection.
We explicitly avoided using DI for DBAL types because they have to stay simple.
We’ve been asked many many times to change this behavior, but doctrine believes that complex data manipulation should NOT happen within the very core of the persistence layer itself. That should be handled in your service layer.

Could you share the link to this one so I could get to know more detail about it?

Why does Dependency Injection assum complexity?

@Legion112 Legion112 changed the title Custom Type which aware of related parameters ValueObject Money custom type Dec 22, 2022
@derrabus
Copy link
Member

All right, I'm closing this issue because all APIs that you want to change are DBAL ones. You're in the ORM repository here.

You might want to contribute to the discussion in doctrine/dbal#2841.

@derrabus derrabus closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2022
@Ocramius
Copy link
Member

I don't have a link, but the type system of DBAL v2 (not sure about v3) uses the flyweight pattern, and is not suited for DI in its form.

@greg0ire
Copy link
Member

Look at the DBAL repo, I've taken steps toward this. Might be on the latest branch, and the work is not finished IIRC

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

No branches or pull requests

4 participants