Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[Form] Addmodel_type option toMoneyType#19359
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mdoutreluingne commentedDec 30, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It seems that the names of the options are arranged in alphabetical order, but I'm not sure,can you confirm this? |
OskarStark commentedDec 30, 2023
Yes |
model_type option toMoneyTypemdoutreluingne commentedDec 31, 2023
Ok, I'll do that in another PR targeting branch |
reference/forms/types/money.rst Outdated
| ..caution:: | ||
| When the value is set to ``integer``, the ``divisor`` option must not be set to ``1``, | ||
| to avoid losing decimals during the casting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The reason looks confusing to me. How is it possible to lose decimals when the divisor is1?
Looking at the implementation, it seems more related to the fact that theinteger value doesn't have any effect when the divisor is1.
mdoutreluingneDec 31, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I misspoke, it is in fact linked to theinteger with a divisor is1.
I find that the caution message is no longer useful. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mdoutreluingne I still don't understand this caution. But it's not because of your PR. I've checked the code and I don't understand it either. I also read this discussion and I still don't understand it:symfony/symfony#51884 (comment)
How would you explain this in a way that anybody can understand it? Thanks.
mdoutreluingneJan 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From what I've understood from looking at the implementation, an exception is thrown if themodel_type isinteger and the divisor is1, because according tothis condition the casting is only done if the divisor is not equal to1, otherwise it only returns the value entered.
Me too, I don't understand why an exception is thrown if the divisor equals1. Casting can be done perfectly well if the divisor is equal to1.
I've added this caution message to warn the reader that if he changes themodel_type forinteger, he must also change thedivisor (because the default divisor value is1), otherwise an exception will be thrown.
But maybe I've added a message that is ultimately of no interest to the reader and the message of the exception is enough.
What do you think?
javiereguiluzJan 3, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I think it's better to not add this to the docs. Developers will get a explict exception message if they do "wrong".
But, maybe we should revisit this or refactor the code to make it easier to understand. It seems that nobody truly understands this code 😐
mdoutreluingneJan 3, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removing caution message done.
I agree, I think it should be refactor the code to make it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've openedsymfony/symfony#53488 to see if we can finally understand the reason of this code. Thanks.
javiereguiluz commentedJan 15, 2024
Thanks Maxime! I've merged this because your contribution is complete and we don't need to wait to fixsymfony/symfony#53488. Thanks. |
Fixes#19353