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] Filter arrays out of scalar form types#29307

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
nicolas-grekas merged 1 commit intosymfony:3.4fromnicolas-grekas:form-array
Dec 8, 2018

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedNov 24, 2018•
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#4102
LicenseMIT
Doc PR-

Replacesfix#20935

lobodol and dmaicher reacted with thumbs up emoji
Copy link
Contributor

@HeahDudeHeahDude left a comment

Choose a reason for hiding this comment

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

Thanks! Finally closing this old issue 🎉

@nicolas-grekasnicolas-grekasforce-pushed theform-array branch 3 times, most recently fromf3d312f to60faaf1CompareNovember 25, 2018 08:51
@nicolas-grekas
Copy link
MemberAuthor

Now with tests, ready.

));
}

publicfunctiontestSubmitWithNonStringDataDoesNotBreakTheFixUrlProtocolListener()
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting test 😄

HeahDude and AntoineLemaire reacted with laugh emoji
@nicolas-grekas
Copy link
MemberAuthor

Why do we need to check for the multiple option here? Such a hard coded knowledge about type specific options looks suspicious to me.

"multiple" is exactly the info we're missing to decide if we allow arrays or not: form types that know how to deal with array of values don't have to be compound, and "multiple" is their way to express they still deal with arrays. Note that this is already what we use, that's why the patch is so simple yet it works and tests are green.

HeahDude and slashfan reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

ro0NL commentedNov 26, 2018•
edited
Loading

Can we check if the type is actual multiple, i.e.getOption('multiple', false) instead ofhasOption('multiple').

Is the same check needed vice versa? If the submitted data is scalar and the type is compound/multiple?

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedNov 26, 2018•
edited
Loading

@ro0NL that's how I first wrote the patch, but the tests broke all around. The reason is that we don't need to decide if arrays are allowed. Instead we need to decide if arraysare dealt with at the form type level. All form types that declare multiple true/false can do that.

ro0NL reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas changed the base branch from2.8 to3.4November 26, 2018 08:42
@nicolas-grekasnicolas-grekas modified the milestones:2.8,3.4Nov 26, 2018
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedDec 7, 2018•
edited
Loading

rebased - ready

@nicolas-grekasnicolas-grekas merged commit000e4aa intosymfony:3.4Dec 8, 2018
nicolas-grekas added a commit that referenced this pull requestDec 8, 2018
…kas)This PR was merged into the 3.4 branch.Discussion----------[Form] Filter arrays out of scalar form types| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#4102| License       | MIT| Doc PR        | -Replacesfix#20935Commits-------000e4aa [Form] Filter arrays out of scalar form types
This was referencedJan 6, 2019
pamil added a commit to Sylius/Sylius that referenced this pull requestJan 8, 2019
…nges (Zales0123)This PR was merged into the 1.2 branch.Discussion----------| Q               | A| --------------- | -----| Branch?         | 1.2| Bug fix?        | yes| New feature?    | no| BC breaks?      | no| Deprecations?   | no| Related tickets | related tosymfony/symfony#29307| License         | MITDue to the latest Symfony 4.1.10/3.4.21 release and changes introduced in linked PR, a bug in select attribute type araised 🌤 Apparently, possible to select values in attribute configuration was saved with `TextType` form type, even though `SelectAttributeValueTranslationsType` should be used. It was somehow parsed before, but know it's not possible and this PR fixes this absurdity 🖖 Commits-------51f7ec2 Fix select attributes according to recent Symfony form changesb80fa00 Fix test in ResourceBundle (taken from#10062)
pamil added a commit to Sylius/SyliusAttributeBundle that referenced this pull requestJan 8, 2019
…nges (Zales0123)This PR was merged into the 1.2 branch.Discussion----------| Q               | A| --------------- | -----| Branch?         | 1.2| Bug fix?        | yes| New feature?    | no| BC breaks?      | no| Deprecations?   | no| Related tickets | related tosymfony/symfony#29307| License         | MITDue to the latest Symfony 4.1.10/3.4.21 release and changes introduced in linked PR, a bug in select attribute type araised 🌤 Apparently, possible to select values in attribute configuration was saved with `TextType` form type, even though `SelectAttributeValueTranslationsType` should be used. It was somehow parsed before, but know it's not possible and this PR fixes this absurdity 🖖 Commits-------51f7ec2481d67270ed66b9879fc57702e4fafb76 Fix select attributes according to recent Symfony form changesb80fa003a4a279c9204465b6189027db50c2285e Fix test in ResourceBundle (taken from #10062)
pamil added a commit to Sylius/SyliusResourceBundle that referenced this pull requestJan 8, 2019
…nges (Zales0123)This PR was merged into the 1.2 branch.Discussion----------| Q               | A| --------------- | -----| Branch?         | 1.2| Bug fix?        | yes| New feature?    | no| BC breaks?      | no| Deprecations?   | no| Related tickets | related tosymfony/symfony#29307| License         | MITDue to the latest Symfony 4.1.10/3.4.21 release and changes introduced in linked PR, a bug in select attribute type araised 🌤 Apparently, possible to select values in attribute configuration was saved with `TextType` form type, even though `SelectAttributeValueTranslationsType` should be used. It was somehow parsed before, but know it's not possible and this PR fixes this absurdity 🖖 Commits-------51f7ec2481d67270ed66b9879fc57702e4fafb76 Fix select attributes according to recent Symfony form changesb80fa003a4a279c9204465b6189027db50c2285e Fix test in ResourceBundle (taken from #10062)
@bvisonl
Copy link

bvisonl commentedJan 9, 2019•
edited
Loading

I was using the type ofTextType::class to submit an array to a data transformer, I am unclear to what could be the alternative to TextType in this scenario in case I need this data to be an array, can anyone provide some guidance?

TrickyC94, fnagel, mayro, sylvaindeloux, jougene, and fatso83 reacted with thumbs up emoji

@TrickyC94
Copy link

TrickyC94 commentedJan 11, 2019•
edited
Loading

I was using the type ofTextType::class to submit an array to a data transformer, I am unclear to what could be the alternative to TextType in this scenario in case I need this data to be an array, can anyone provide some guidance?

I'm on the same situation. Things worked before this update

@omarboussarsar
Copy link

omarboussarsar commentedApr 25, 2019•
edited
Loading

This code can help you to bind an array with a TextType by serializing the array before the submit.
(In my case I need to serialize all the children of my form type).

$builder->addEventListener(FormEvents::PRE_SUBMIT,function (FormEvent$event) {$event->setData(array_map(function ($value) {returnserialize($value);    },$event->getData()));})
fatso83 reacted with thumbs up emoji

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

Reviewers

@xabbuhxabbuhxabbuh requested changes

@stofstofstof approved these changes

+4 more reviewers

@ro0NLro0NLro0NL approved these changes

@TobionTobionTobion approved these changes

@dmaicherdmaicherdmaicher approved these changes

@HeahDudeHeahDudeHeahDude approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

15 participants

@nicolas-grekas@ro0NL@bvisonl@TrickyC94@omarboussarsar@vkurdin@stof@Tobion@ewgRa@dmaicher@quantizer@apfelbox@xabbuh@HeahDude@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp