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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:form-array branch from b7da2a5 to 8281731 Nov 24, 2018

@HeahDude
Copy link
Member

HeahDude left a comment

Thanks! Finally closing this old issue 🎉

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:form-array branch 3 times, most recently from f3d312f to 60faaf1 Nov 24, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 25, 2018

Now with tests, ready.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:form-array branch from 60faaf1 to f78eaa3 Nov 25, 2018

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

This comment has been minimized.

@dmaicher

dmaicher Nov 25, 2018

Contributor

Interesting test 😄

@Tobion

Tobion approved these changes Nov 25, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 25, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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.

@ro0NL

ro0NL approved these changes Nov 26, 2018

@nicolas-grekas nicolas-grekas changed the base branch from 2.8 to 3.4 Nov 26, 2018

@nicolas-grekas nicolas-grekas modified the milestones: 2.8, 3.4 Nov 26, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:form-array branch from f78eaa3 to c377561 Nov 26, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 26, 2018

Now targeting 3.4, let 2.8 in peace.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:form-array branch from c377561 to d7faca2 Nov 26, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:form-array branch from d7faca2 to 000e4aa Dec 7, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 7, 2018

rebased - ready

@stof

stof approved these changes Dec 8, 2018

@nicolas-grekas nicolas-grekas merged commit 000e4aa into symfony:3.4 Dec 8, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 8, 2018

bug #29307 [Form] Filter arrays out of scalar form types (nicolas-gre…
…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

bug #10077 Fix select attributes according to recent Symfony form cha…
…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

bug #10077 Fix select attributes according to recent Symfony form cha…
…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

bug #10077 Fix select attributes according to recent Symfony form cha…
…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)
@quantizer

This comment has been minimized.

Copy link

quantizer commented on src/Symfony/Component/Form/Form.php in 000e4aa Jan 8, 2019

@nicolas-grekas hello,

I’m using the form for JSON validation in my project, so I do json decode of request content and do form->submit with this array. But after this update one of my fields, which array of arrays failed with Submitted data was expected to be text or number, array given.

Here is an example of what I try to set in a field:

"products_quantity" => [                                            
        [                                                             
            "product_id": "ID",     
            "quantity": 10                                            
        ],                                                            
        [                                                             
            "product_id": "ID",     
            "quantity": 3                                             
        ]                                                             
    ],

I tried to use choice field type with multiple true parameter. But values could be only strings or numbers. Because this is feature of submit method custom field type and transformers won't help in this situation. So looks like I need to change input format?

This comment has been minimized.

Copy link
Member

nicolas-grekas replied Jan 8, 2019

That's correct.

This comment has been minimized.

Copy link
Contributor

ewgRa replied Jan 18, 2019

Same problem here. We try to send an array to $builder->add('settings')

Looks like broken backward compatibility. How now we can achieve it?

This comment has been minimized.

Copy link
Member

xabbuh replied Jan 18, 2019

please read #29809, #29841, #29905 and #29911

@bvisonl

This comment has been minimized.

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

This comment has been minimized.

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

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