Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] [MoneyType] Add "input" option#50720
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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| 'currency' =>'EUR', | ||
| 'compound' =>false, | ||
| 'html5' =>false, | ||
| 'scale'=>2, |
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 codestyle changes have to be reverted
| $this->assertSame(12345,$form->getData()); | ||
| } | ||
| publicfunctiontestInputTypeIntegerAndDivisorNotEgalToOne() |
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.
| publicfunctiontestInputTypeIntegerAndDivisorNotEgalToOne() | |
| publicfunctiontestInputTypeIntegerAndDivisorNotEqualToOne() |
| publicfunctiontestValueToIntegerWithSpecificOptionInputToInteger() | ||
| { | ||
| $form =$this->factory->create(static::TESTED_TYPE,null, ['input' =>'integer']); | ||
| $form->submit('12345.6'); |
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.
IMO this case should trigger an exception in duringreverseTransform()
| publicfunctiontestHtml5EnablesSpecificFormattingWithIntegerFormat() | ||
| { | ||
| $form =$this->factory->create(static::TESTED_TYPE,null, ['html5' =>true,'scale' =>2,'input' =>'integer']); |
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 am not sure that a scale option with a value other than0 plays well with how theinput option is implemented currently.
But, this makes me wonder if the way this option is currently implemented actually makes sense. So maybe we should indeed allow thedivision to be greater than 1 and take it into account to decide whether we want to render anumber or aninteger widget.
javiereguiluz left a comment
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 like this feature, but I wonder about the new option name:input sounds too generic.
Just wondering: since we have other options calledmodel_* (e.g.model_timezone in nhttps://symfony.com/doc/current/reference/forms/types/datetime.html#model-timezone). Should we rename this option to follow that pattern? Some ideas:
model_formattingmodel_formatmodel_data_type...fabpot commentedOct 1, 2023
@BASAKSemih Do you have time to take comments into account? |
BASAKSemih commentedOct 5, 2023
No sorry i don't have time |
fabpot commentedOct 5, 2023
Thank you for your answer. Let’s close for now then. Maybe someone will take over. |
This PR was squashed before being merged into the 7.1 branch.Discussion----------[Form] add "model_type" option to MoneyType| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets |Fix#50707| License | MITThis is a replacement for abandoned#50720Please notice that the previous PR misunderstood the related issue: the intended casting should be applied when the divisor is not 1, otherwise it doesn't make sense.Commits-------8811741 [Form] add "model_type" option to MoneyType
Uh oh!
There was an error while loading.Please reload this page.