Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Form] fixed BC break with pre selection of choices withChoiceType and its children#18180

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

Merged
fabpot merged 2 commits intosymfony:2.7fromHeahDude:fix-form-radio_list_mapper
Apr 28, 2016

Conversation

@HeahDude
Copy link
Contributor

QA
Branch2.7+
Bugfix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#18173,#14712,#17789
LicenseMIT
Doc PR-

ChoiceType now always useChoiceToValueTransformer orChoicesToValuesTransformer depending onmultiple option.
HenceCheckboxListMapper andRadioListMapper don't handle the transformation anymore.

$form->submit('');

$this->assertNull($form->getData());
$this->assertEmpty($form->getData());
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This looks like a collateral fix to me :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@webmozart you need to read this test in particular, the others are just using an empty string instead ofnull asviewData but this one concerns the data.

Also this PR fixes the failed tests added originally in#17760 (refhttps://github.com/symfony/symfony/pull/17760/files#diff-533d45d8bb41e489890ad12df7a1a4b7R163).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This time it just doesn't break the data transformers call :)

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch 2 times, most recently from137bf44 to5cc6bb4CompareMarch 15, 2016 16:55
@javiereguiluz
Copy link
Member

@HeahDude thanks for this fix (and for everything you do for the Form component).

Just a minor comment: it seems that the| Bug fix? | yes/no row disappeared from the PR description table (it is usually displayed after theBranch? row). This information is important for mergers. Could you please include it? Thanks!

@HeahDude
Copy link
ContributorAuthor

@javiereguiluz done :)

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch from5cc6bb4 to76e265cCompareMarch 15, 2016 17:19
HeahDude referenced this pull requestMar 16, 2016
This fixes "false" choice pre selection when `ChoiceType`is `expanded` and not `multiple`
@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch from76e265c to01b8101CompareMarch 16, 2016 10:24
*/
publicfunctionmapDataToForms($data,$radios)
publicfunctionmapDataToForms($choice,$radios)
{
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Should I test null of force a string casting before throwing here ?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@webmozart I re-check the code and here the view data should always be a string.

In contrary, sincemapsFormsToData() happens before transformation we should acceptnull there as well.

Do you agree or am I missing something ? Thanks.

@HeahDude
Copy link
ContributorAuthor

Failures seem unrelated.

@nakashu
Copy link

👍

foreach ($checkboxesas$checkbox) {
$value =$checkbox->getConfig()->getOption('value');
$checkbox->setData(isset($valueMap[$value]) ?true :false);
$checkbox->setData(in_array($value,$choices,true) ?true :false);
Copy link
Member

Choose a reason for hiding this comment

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

the ternary operator turning a boolean into the same boolean looks weird to me

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@stof I first felt the same, as you can see I did to match the previous style (in both checkbox and radio list mappers).

I guess it's a kind of explicit casting since this is where choices string values are converted to their boolean "selected" state values.

Thanks for the review.

Copy link
Contributor

Choose a reason for hiding this comment

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

But technically

in_array($value,$choices,true) ?true :false

is the same as

in_array($value,$choices,true)

or am I missing something? :)

Copy link
Member

Choose a reason for hiding this comment

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

I would indeed remove those unneeded ternary operators when the left side is already a bool.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fair enough then.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch 2 times, most recently fromaa6fae7 to695b5b1CompareMarch 17, 2016 13:09
?newCheckboxListMapper($options['choice_list'])
:newRadioListMapper($options['choice_list']));
?newCheckboxListMapper()
:newRadioListMapper());
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Little detail, it's now short enough to be inlined here imho, isn't it ?

Copy link
Member

Choose a reason for hiding this comment

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

It could have been inlined before as well :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

done :)

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch 2 times, most recently from3f2b402 to103566eCompareMarch 17, 2016 13:35
@HeahDude
Copy link
ContributorAuthor

Comments addressed.

@HeahDude
Copy link
ContributorAuthor

TwigBridge dependency toEventDispatcher seems to be broken with php 7 build in travis.

@xabbuh
Copy link
Member

@HeahDude Can you check if rebasing fixes the tests?

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch from103566e to58e8ed0CompareMarch 19, 2016 12:09
@HeahDude
Copy link
ContributorAuthor

@xabbuh It did. Thanks!

}
}elseif ($options['multiple']) {
// <select> tag with "multiple" option
if ($options['multiple']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand why this is necessary. It seems to me the transformer and the data mappers are doing the same thing already. Can you add some explanation?

$e->getCode(),
$e
);
thrownewUnexpectedTypeException($choices,'array');
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

same here

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch from58e8ed0 tob3fa216CompareApril 1, 2016 08:51
@HeahDude
Copy link
ContributorAuthor

Ok there was a reason why I useassertEmpty, it's because$extraData in as empty array while$viewData is an empty string.

I fixed the assertions accordingly.

Rebased and squashed! Needs a final review here. Thanks!

@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch fromb3fa216 toc77ae14CompareApril 1, 2016 09:12
@HeahDude
Copy link
ContributorAuthor

There is a little failure in Doctrine bridge because it needs that PR to be merged first.

I've updated the dependency to2.7.12 but travis does not seem to be aware of it yet.

Is it OK ?

@HeahDudeHeahDude changed the title[Form] fix pre selection of choices withChoiceType[Form] [BC Break] fix pre selection of choices withChoiceTypeApr 1, 2016
@HeahDudeHeahDude changed the title[Form] [BC Break] fix pre selection of choices withChoiceType[Form] fixed BC break with pre selection of choices withChoiceType and its childrenApr 1, 2016
"symfony/stopwatch":"~2.2",
"symfony/dependency-injection":"~2.2",
"symfony/form":"~2.7,>=2.7.1",
"symfony/form":"~2.7,>=2.7.12",
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 try~2.7.12|~2.8.5 please?

@HeahDude
Copy link
ContributorAuthor

@xabbuh Done, let's wait & see :)

@HeahDude
Copy link
ContributorAuthor

Nope :(

@HeahDude
Copy link
ContributorAuthor

@xabbuh Can this be related to the last releases messed tags ?

@xabbuh
Copy link
Member

@HeahDude This looks good now (thedeps=low job is passing).

@HeahDude
Copy link
ContributorAuthor

Great!

@dmaicher
Copy link
Contributor

Any news here? Waiting for this fix so I can upgrade to Symfony 2.8.* :)

peternijssen, johanderuijter, and flipspl reacted with thumbs up emoji

fixessymfony#14712 andsymfony#17789.`ChoiceType` now always use `ChoiceToValueTransformer` or`ChoicesToValuesTransformer` depending on `multiple` option.Hence `CheckboxListMapper` and `RadioListMapper` don’t handlethe transformation anymore.Fixes pre selection of choice with model values such as `null`,`false` or empty string.
@HeahDudeHeahDudeforce-pushed thefix-form-radio_list_mapper branch from9d2dc37 toea5375cCompareApril 9, 2016 13:29
@fabpot
Copy link
Member

Thank you@HeahDude.

@fabpotfabpot merged commitea5375c intosymfony:2.7Apr 28, 2016
fabpot added a commit that referenced this pull requestApr 28, 2016
…ChoiceType` and its children (HeahDude)This PR was merged into the 2.7 branch.Discussion----------[Form] fixed BC break with pre selection of choices with `ChoiceType` and its children| Q             | A| ------------- | ---| Branch        | 2.7+| Bugfix?  | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#18173,#14712,#17789| License       | MIT| Doc PR        | --f7eea72 reverts BC break introduced in#17760-58e8ed0 fixes pre selection of choice with model values such as `false`, `null` or empty string without BC break.`ChoiceType` now always use `ChoiceToValueTransformer` or `ChoicesToValuesTransformer` depending on `multiple` option.Hence `CheckboxListMapper` and `RadioListMapper` don't handle the transformation anymore.Commits-------ea5375c [Form] refactor CheckboxListMapper and RadioListMapper71841c7 Revert "[Form] refactor `RadioListMapper::mapDataToForm()`"
@HeahDudeHeahDude deleted the fix-form-radio_list_mapper branchApril 28, 2016 10:31
HeahDude added a commit to HeahDude/symfony that referenced this pull requestApr 28, 2016
fabpot pushed a commit that referenced this pull requestApr 28, 2016
fabpot added a commit that referenced this pull requestApr 28, 2016
This PR was submitted for the master branch but it was merged into the 3.0 branch instead (closes#18663).Discussion----------[Form] Fix tests added in#18180| Q             | A| ------------- | ---| Branch?       | 3.0+| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | ~| License       | MIT| Doc PR        | ~Commits-------c45a435 [Form] Fix tests added in#18180
fabpot added a commit that referenced this pull requestApr 28, 2016
* 3.0:  [Form] Fix tests added in#18180
@HeahDude
Copy link
ContributorAuthor

@dmaicher, happy upgrade! :)

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@HeahDude@javiereguiluz@nakashu@xabbuh@dmaicher@fabpot@webmozart@stof@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp