Movatterモバイル変換

Skip to content

symfony/symfonyPublic

NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Form] add "model_type" option to MoneyType#51884

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

fabpot merged 1 commit intosymfony:7.1fromgarak:moneytype_new_option

Dec 29, 2023

Merged

[Form] add "model_type" option to MoneyType#51884

fabpot merged 1 commit intosymfony:7.1fromgarak:moneytype_new_option

Dec 29, 2023

Conversation

@garak

Copy link

Contributor

garak commentedOct 7, 2023•
edited by fabpot
Loading

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#50707
License	MIT

This is a replacement for abandoned#50720

Please 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.

@garak

garak requested review fromxabbuh andyceruto ascode owners

October 7, 2023 16:51

@carsonbot

carsonbot added Status: Needs Review Feature labels

@carsonbot

carsonbot added this to the6.4 milestone

@garak

garak force-pushed themoneytype_new_option branch fromeae8a80 todd0c07dCompare

October 7, 2023 17:10

@OskarStark

OskarStark added the Form label

@carsonbot

carsonbot changed the title~~add "model_type" option to MoneyType~~[Form] add "model_type" option to MoneyType

yceruto

yceruto requested changes

View reviewed changes

Copy link

Member

yceruto left a comment

Choose a reason for hiding this comment

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

Thanks Massimiliano for this improvement! I left a minor request.

src/Symfony/Component/Form/Extension/Core/DataTransformer/MoneyToLocalizedStringTransformer.phpShow resolvedHide resolved

@carsonbot

carsonbot added Status: Needs Work and removed Status: Needs Review labels

yceruto

yceruto approved these changes

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/MoneyType.php OutdatedShow resolvedHide resolved

@carsonbot

carsonbot added Status: Reviewed and removed Status: Needs Work labels

@yceruto

yceruto added the Ready label

@garak

Copy link

ContributorAuthor

garak commentedOct 29, 2023

@xabbuh could you review, please?

@nicolas-grekas

nicolas-grekas modified the milestones:6.4,7.1

nicolas-grekas

nicolas-grekas reviewed

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Please add a line in the chagelog of the component in section 7.1

src/Symfony/Component/Form/Extension/Core/Type/MoneyType.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Form/Extension/Core/Type/MoneyType.php Outdated


		$resolver->setNormalizer('model_type',staticfunction (Options$options,$value) {
		if ('integer' ===$value &&1 ===$options['divisor']) {
		thrownewLogicException('Can use integer "model_type" option only when the "divisor" option is not 1.');

Copy link

Member

nicolas-grekasNov 20, 2023

Choose a reason for hiding this comment

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

Suggested change

	thrownewLogicException('Can use integer"model_type" optiononly whenthe "divisor" optionis not1.');
	thrownewLogicException('When the"model_type" optionis set to "integer",the "divisor" optionshould notbe set to "1".');

but why?

Copy link

ContributorAuthor

garakNov 25, 2023

Choose a reason for hiding this comment

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

Because if you set the divisor to 1, you can end up with a float instead of an integer, and lose decimals in the casting

Copy link

Member

nicolas-grekasNov 25, 2023

Choose a reason for hiding this comment

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

I'm sorry I don't get it. Also the original PR had exactly the opposite : throwing when the divisor isnot 1.
https://github.com/symfony/symfony/pull/50720/files#diff-eb30dc48f2b749fa5d76ce9598bb51c00fcb97bd7a10337f0630db00f5ba3b9aR100

I'm pretty sure I'm missing something 😬

Copy link

ContributorAuthor

garakNov 25, 2023

Choose a reason for hiding this comment

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

I know, and indeed I added a note about that when I opened this PR.

src/Symfony/Component/Form/Tests/Extension/Core/Type/MoneyTypeTest.phpShow resolvedHide resolved

@garak

Copy link

ContributorAuthor

garak commentedNov 25, 2023

Please add a line in the chagelog of the component in section 7.1

I see no changelog for 7.1, should I create a new file?

@nicolas-grekas

Copy link

Member

nicolas-grekas commentedNov 25, 2023

I mean this file:https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Form/CHANGELOG.md

fabpot

fabpot requested changes

View reviewed changes

src/Symfony/Component/Form/CHANGELOG.mdShow resolvedHide resolved

@carsonbot

carsonbot added Status: Needs Work and removed Status: Reviewed labels

fabpot

fabpot approved these changes

View reviewed changes

@carsonbot

carsonbot added Status: Reviewed and removed Status: Needs Work labels

@garak

@fabpot

[Form] add "model_type" option to MoneyType

8811741

@fabpot

fabpot force-pushed themoneytype_new_option branch frome5dfc53 to8811741Compare

December 29, 2023 07:55

@fabpot

Copy link

Member

fabpot commentedDec 29, 2023

Thank you@garak.

@fabpot

fabpot merged commitf5d993b intosymfony:7.1

@fabpot

fabpot mentioned this pull request

[Form] add "model_type" option to MoneyTypesymfony/symfony-docs#19353

Closed

@garak

garak deleted the moneytype_new_option branch

December 29, 2023 09:40

@javiereguiluz

javiereguiluz mentioned this pull request

[Form] Addmodel_type option toMoneyTypesymfony/symfony-docs#19359

Merged

@javiereguiluz

javiereguiluz mentioned this pull request

[Form] Issue with some code inMoneyType#53488

Closed

@fabpot

fabpot mentioned this pull request

Release v7.1.0-BETA1#54820

Merged

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Labels

Feature Form Ready Status: Reviewed

6 participants

@garak

@nicolas-grekas

@fabpot

@yceruto

@OskarStark

@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp