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

[Form] weaken the rejection of submitted array data#29911

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

Jump to bottom

Closed

xabbuh wants to merge1 commit intosymfony:3.4fromxabbuh:issue-29809

Closed

[Form] weaken the rejection of submitted array data#29911

xabbuh wants to merge1 commit intosymfony:3.4fromxabbuh:issue-29809

Conversation

Copy link

Member

xabbuh commentedJan 16, 2019•
edited
Loading

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#29809,#29841,#29905
License	MIT
Doc PR

It seems that the bugfix made in#29307 seems to break quite some applications. I therefore suggest that we solve the issue a bit different by introducing a newaccept_multiple_values option. This value will benull by default which means no checks will be performed for backwards compatibility. The core form types do explicitly set this value tofalse if they expect the submitted data to be strings. If a form is compound, the option value will automatically default totrue.

Since from my experience most custom form types have child forms, they are already compound and so do accept arrays. Nothing will change for them. Other custom types will not have the check as long as they do not extend one of the built-in types that have this check enabled.

In Symfony 4.3 we can then deprecate thenull default value so that each type must opt for eithertrue orfalse as the value in 5.0.

xabbuh added this to the3.4 milestone

Jan 16, 2019

carsonbot added Status: Needs Review Form Bug labels

Jan 16, 2019

xabbuh force-pushed theissue-29809 branch from43ecc11 tod51aea3Compare

January 16, 2019 22:09

xabbuh commented

Jan 16, 2019

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

		'error_bubbling' =>false,
		'compound' =>$compound,
		// submitted arrays are dealt with in data transformers
		'accept_multiple_values' =>true,

Copy link

MemberAuthor

xabbuhJan 16, 2019

Choose a reason for hiding this comment

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

Not sure if that's the best name. I could also imagine something likeaccept_array_data.

Copy link

YetiCGNJan 17, 2019•
edited
Loading

Choose a reason for hiding this comment

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

Suggestion:accept_unstructured_data? "Array" could be mistaken for a flat array without special string keys, just like multiple values on aTextType.

Copy link

asentnerApr 9, 2019

Choose a reason for hiding this comment

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

'scalar' => false?

This was referencedJan 16, 2019

Form Validation Error for Array input#29905

Closed

FormType mapping for array on TextType not longer working#29809

Closed

Copy link

Member

nicolas-grekas commentedJan 17, 2019

Will that make the reported breaks work again? Eg submitting a json-decoded array in a TextType?
I'm not sure we should do this for now: input data should always be strictly validated. Allowing arrays was a really nasty behavior, on the edge of a security risk maybe for some.

Copy link

Contributor

OskarStark commentedJan 17, 2019

bugfix made in # seems to break

which PR are you talking about? 🤔

Copy link

Contributor

althaus commentedJan 17, 2019•
edited
Loading

@OskarStark See#29809 for the main discussion triggered by PR#29307 .

Copy link

Contributor

althaus commentedJan 17, 2019

The core form types do explicitly set this value to false if they expect the submitted data to be strings.

What'd the behaviour of thenull form type which defaults to TextType? My reading is that those "evil" forms are still broken?

Copy link

MemberAuthor

xabbuh commentedJan 17, 2019

@nicolas-grekas As long as you do not explicitly set theaccept_multiple_values totrue yourself array values would still be rejected (that's why we need to be explicit about this in the core form types).

But what this will allow easily again having a custom type like this:

class UnstructuredTypeextends AbstractType{}

@althaus Yes, if you are not explicitly about the type being used and theTextType is the result of the guessing process, this PR does not change anything for you.

Copy link

Contributor

alexander-schranz commentedJan 17, 2019•
edited
Loading

@xabbuh if I'm interpereting it correctly I could use for unstructured data this then:

useSymfony\Component\Form\Extension\Core\Type\FormType;$builder->add('some_unstructured_data', FormType::class');

Copy link

MemberAuthor

xabbuh commentedJan 17, 2019

@alexander-schranz Yes, that should work (that basically is the same as this test:https://github.com/symfony/symfony/pull/29911/files#diff-de65be85a5affed1f499b79d96965419R1062). But I'd be happy if you could confirm that in your application context.

alexander-schranz reviewed

Jan 17, 2019

View reviewed changes

src/Symfony/Component/Form/Tests/CompoundFormTest.php Outdated

		$this->form->add($this->getBuilder('foo',null,null, ['compound' =>true])->getForm());

		$this->form->submit([
		'foo' => ['foo'],

Copy link

Contributor

alexander-schranzJan 17, 2019•
edited
Loading

Choose a reason for hiding this comment

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

would also add test for a nested array not just a list:

['foo' => ['foo' =>'foo','bar' => ['baz' =>'baz',    ]  ]]

@xabbuh

Copy link

Contributor

alexander-schranz commentedJan 17, 2019

@xabbuh will test it in the application at the evening. Thank you for having a look at the issue!

dpfaffenbauer mentioned this pull request

Jan 17, 2019

"settings: this value is not valid" error when saving new Executabledpfaffenbauer/ProcessManager#28

Closed

weaken the rejection of submitted array data

3ea6f14

xabbuh force-pushed theissue-29809 branch fromd51aea3 to3ea6f14Compare

January 17, 2019 21:05

Copy link

MemberAuthor

xabbuh commentedJan 17, 2019

@alexander-schranz makes sense, done

xabbuh referenced this pull request

Jan 18, 2019

[Form] Filter arrays out of scalar form types

000e4aa

alexander-schranz reviewed

Jan 20, 2019

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/TextType.php

		{
		$resolver->setDefaults([
		'compound' =>false,
		'accept_multiple_values' =>false,

Copy link

Contributor

alexander-schranzJan 20, 2019

Choose a reason for hiding this comment

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

@xabbuh this should set to true until 5.0 to avoid the bc break which was introduce in 3.4.21, else applications which did work before 3.4.21 still would crash.

Copy link

Member

nicolas-grekas commentedJan 21, 2019

I'm not sure at all we should do this change.
The behavior change we introduced is a bug fix.
I'm sorry some "hacked" the component and are caught by the fix, but every bug fix does that potentially.
Does this PR provide something that cannot be achieved otherwise?
Adding a new option means added complexity for everyone, better think twice about it.

Copy link

Contributor

althaus commentedJan 21, 2019

I'm sorry some "hacked" the component

I'm pretty sure that 9 out of 10 didn't do this on purpose, but either by accident or due to lacking docs back then. The Form component really can be a b**ch sometimes. It's the only one forcing us to rework code parts every now and then due to some "fix". :/

I can also see that adding a fire-and-forget option just for this small thing on all those classes could raise more headache than it solves. So I'd be glad if someone can imagine a solution working for both sides.