Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] Filter arrays out of scalar form types #29307

Merged
merged 1 commit into from Dec 8, 2018

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 24, 2018

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 -

Replaces fix #20935

Copy link
Contributor

@HeahDude HeahDude 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-grekas nicolas-grekas force-pushed the form-array branch 3 times, most recently from f3d312f to 60faaf1 Compare November 25, 2018 08:51
@nicolas-grekas
Copy link
Member Author

Now with tests, ready.

@@ -93,14 +93,6 @@ public function testThrowExceptionIfDefaultProtocolIsInvalid()
));
}

public function testSubmitWithNonStringDataDoesNotBreakTheFixUrlProtocolListener()
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 😄

@nicolas-grekas
Copy link
Member Author

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.

@ro0NL
Copy link
Contributor

ro0NL commented Nov 26, 2018

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

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

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Nov 26, 2018

@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 arrays are dealt with at the form type level. All form types that declare multiple true/false can do that.

@nicolas-grekas nicolas-grekas changed the base branch from 2.8 to 3.4 November 26, 2018 08:42
@nicolas-grekas nicolas-grekas modified the milestones: 2.8, 3.4 Nov 26, 2018
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 7, 2018

rebased - ready

@nicolas-grekas nicolas-grekas merged commit 000e4aa into symfony:3.4 Dec 8, 2018
nicolas-grekas added a commit that referenced this pull request Dec 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        | -

Replaces fix #20935

Commits
-------

000e4aa [Form] Filter arrays out of scalar form types
This was referenced Jan 6, 2019
pamil added a commit to Sylius/Sylius that referenced this pull request Jan 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 to symfony/symfony#29307
| License         | MIT

Due 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 changes
b80fa00 Fix test in ResourceBundle (taken from #10062)
pamil added a commit to Sylius/SyliusAttributeBundle that referenced this pull request Jan 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 to symfony/symfony#29307
| License         | MIT

Due 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 changes
b80fa003a4a279c9204465b6189027db50c2285e Fix test in ResourceBundle (taken from #10062)
pamil added a commit to Sylius/SyliusResourceBundle that referenced this pull request Jan 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 to symfony/symfony#29307
| License         | MIT

Due 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 changes
b80fa003a4a279c9204465b6189027db50c2285e Fix test in ResourceBundle (taken from #10062)
@bvisonl
Copy link

bvisonl commented Jan 9, 2019

I was using the type of TextType::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
Copy link

TrickyC94 commented Jan 11, 2019

I was using the type of TextType::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 commented Apr 25, 2019

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) {
        return serialize($value);
    }, $event->getData()));
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet