[Form] Fix submitting checkboxes and radios with PATCH method #17771

HeahDude · 2016-02-11T17:28:28Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#16802, #17799, #17899, #20179
License	MIT
Doc PR	-

Confirm this is the right fix
Update tests for 2.8 with FQCN
Update tests for 3.0 by flipping choice keys and values

The bug seems to make it impossible to submit an expanded ChoiceType with a PATCH method, see #17799.

ostrolucky · 2016-02-15T12:42:16Z

src/Symfony/Component/Form/Form.php

@@ -580,6 +580,8 @@ public function submit($submittedData, $clearMissing = true)
                        if (method_exists($child, 'getClickedButton') && null !== $child->getClickedButton()) {
                            $this->clickedButton = $child->getClickedButton();
                        }
+                    } elseif (!$isSubmitted && !$clearMissing && array_key_exists($name, $submittedData)) {


This never evaluates to TRUE, because $isSubmitted = array_key_exists($name, $submittedData);

I know, this is a dummy test, see HeahDude@935b2da#diff-ca5e25b47f3ecc94cd557946aeb486c6R583 for the right fix

fabpot · 2016-02-17T06:20:28Z

src/Symfony/Component/Form/Form.php

@@ -562,6 +562,7 @@ public function submit($submittedData, $clearMissing = true)

                foreach ($this->children as $name => $child) {
                    $isSubmitted = array_key_exists($name, $submittedData);
+                    $clearMissing = $this->getConfig()->getOption('expanded') ?: $clearMissing;


expanded being an option for ChoiceType, it looks weird to "hardcode" this condition here.

@fabpot I have the same feeling, but I miss to find another way... expanded choice type seems to be a really singular case.

Feel free to close this PR if there is a better fix.

@fabpot unless I missed something else, $clearMissing in only used as an argument of FormType::submit() and there is no way no access its behaviour from a type definition, or it would need to override this method.

Handling a custom submit process for a particular form type would require a big refactoring...

@webmozart any thoughts ?

a "no fix" is still a solution, but the doc should mention that using a PATCH method is not handled by ChoiceType

HeahDude · 2016-02-23T12:55:27Z

Ok I've updated the fix as it comes from the CheckboxType since the ChoiceType got nested ones when expanded and multiple.

Silverspur · 2016-02-23T13:40:41Z

I'm unsure this new fix is acceptable because it leads to the opposite problem (if I'm not mistaken):
Let's look at an entity with two boolean attributes b1 and b2. Currently, b1 and b2 are true. If now there is a form intended to only manage b1, that is sumbitted with the checkbox unchecked, then b1 will flip to false (this is what the fix corrects, this is the expected behaviour) but b2 will also flip to false even though the form is not supposed to affect it.

HeahDude · 2016-02-23T13:50:06Z

@Silverspur It wouldn't if there is no field for that b2 property which should be the case when a form is not supposed to manage it.

Silverspur · 2016-02-23T14:05:41Z

You're perfectly right, I got confused. My bad.

HeahDude · 2016-02-23T14:28:08Z

Maybe I should add the clear_missing option in the BaseType with a default false value, so it can be overwritten "internally" for nested types needing to force update like CheckboxType to make this fix global and consistent.

@webmozart, could you confirm ?

Silverspur · 2016-02-23T16:37:24Z

@HeahDude Actually the clear_missing option should be also set to true by default for multiple+expanded 'choice' fields. Otherwise, when submitting the form with zero checkbox checked in the choice field, nothing will happen.

I still find odd the fact that the data related to a field present in the form type but with no data sent will :

not be updated if it is, say, an integer (which is what I expect, and is the whole reason why $clearMissing is set to false for PATCH requests)
be updated to false if this is a checkbox.

I know the root of this issues comes from the way HTTP forms are designed, but I still find this discrepancies quite unconsistent, and I think it somehow questions the pertinence of systematically setting $clearMissing to false for PATCH, since the setting will only apply to certain types of fields.

HeahDude · 2016-02-23T20:06:02Z

@Silverspur

a field present in the form type but with no data sent will not be updated

I don't agree with you as it actually depends on the mandatory $clearMissing to know what to do with it.
Either is will update it by unsetting the previous data (new data = null when field is missing or has an empty string value) OR it will ignore it (which is the need of the PATCH method). This is precisely the meaning of this option and you're right when you say this is because of the way the HTTP is designed.

Actually the clear_missing option should be also set to true by default for multiple+expanded 'choice' fields

This was the misinterpretation of my first fix, but it's not true as we found out thanks to your issue ;)
There is no need (tests speak for themselves) to set this option for the ChoiceType as this new fix is applied on submitted children forms (see https://github.com/symfony/symfony/pull/17771/files#diff-ca5e25b47f3ecc94cd557946aeb486c6R567)

HeahDude · 2016-02-24T09:20:52Z

Ok so I may need to clarify the patch I propose here, as I really think this time this is the way to fix it, at least for keeping BC in 2.7.

#16802 and #17799 pointed out that the expanded ChoiceType was not updated with a PATCH method because $clearMissing is false is that case.

Then thanks to #17899, I understood that for once ChoiceType is not in cause.

A PATCH method would fail for any nested CheckboxType in a form like RadioType which extends it.
This is their particularity since in contrast with other types they don't map a "property" field (model data) but more accurately the state of a field as a virtual boolean as they might not been sent via HTTP when (virtually) "false".

So to fix this in a BC way for 2.7, we need IMO this "hack" as the clear_missing option is a kind of a feature which should only be used internally. But maybe we should add a note about it in the reference of CheckboxType and set it as default false in BaseType to handle very edge cases.

What I suggest from here, is to revert this change in master and properly implement this (BC break) by adding FormConfigInterface::getClearMissing() and document it as well.

Another option (still BC) for master would be to introduce a (possibly abstract) BooleanType which would hold the clear_missing option and make CheckboxType extending it.

What do you think symfony deciders ?

HeahDude · 2016-02-25T09:33:12Z

Actually this is the same case for the compound option as said in the docs as of 2.3 :

    compound
    ~~~~~~~~

    **type**: ``boolean`` **default**: ``false``

    This option specifies whether the type contains child types or not. This option
    is managed internally for built-in types, so there is no need to configure
    it explicitly.

fabpot · 2016-06-14T06:59:12Z

@HeahDude I can see a list of task in the PR description. Are they still accurate? Anything we can do to move this PR forward? Thanks.

HeahDude · 2016-06-15T07:22:25Z

@fabpot Yes I guess they are, having a clear_missing option would also help dealing with #18998 with more control from disabled option IMHO.

I waited for some feedback, I'm not sure if this is acceptable in 2.7. Any thoughts?

fabpot · 2016-07-01T05:44:33Z

Looks like a bug fix to me, so 2.7 seems fine.

HeahDude · 2016-12-03T14:07:23Z

Ok this one is finally ready 🎉, had the chance to talk about this with @webmozart in person during the Hack Day in SymfonyCon Berlin, and we agreed on keeping this internal for now.

I've just renamed the option force_submit and use it only when there initial data is set but missing on submission, so PATCH method now works for nested checkboxes and radios, this allows ChoiceType to work with PATCH too.

HeahDude · 2016-12-03T15:18:54Z

status: needs work

Simperfit · 2016-12-09T18:36:32Z

Great work @HeahDude !

ostrolucky · 2016-12-11T09:14:44Z

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

@@ -64,7 +64,7 @@ public function configureOptions(OptionsResolver $resolver)

        $resolver->setNormalizer('force_submit', function (Options $options) {
            // If pre set data is true, we need to ensure that $emptyData will be submitted
-            return (bool) $options['data'];
+            return isset($options['data']) && (bool) $options['data'];


how about

return !empty($options['data']);

instead?

xabbuh · 2016-12-18T08:40:06Z

@HeahDude What is the state here?

HeahDude · 2016-12-18T09:39:56Z

@xabbuh I'm still quite reluctant to merge this fix as is, as it makes impossible to ignore checkboxes or radios with PATCH request (they will always be submitted).

I think I should reduce the scope of this PR to act only on parent forms containing such fields like the ChoiceType (which is also buggy with POST), then better document the problem and its solutions, and continue the work on this as a feature on master.

I mean actually problems with PATCH method are quite expected and the component should handle only two cases IMHO:

The form contains every possible fields to update:
- the view is in html and will bug when submitting some checkboxes or radio, adding hidden input should be the way to go (but is not the component responsibility though), this case does not really make sense to me (cf point 2), excepted when it's about compound forms like expanded ChoiceType.
- the data is an array (i.e got from some json data), name should be explicit and empty strings should be used to update any fields to null (or false for boolean types, the conversion is already handled by the component, there are neither report nor bug for this case).
The form contains a subset of the possible fields to update which is actually defining the PATCH context:
- the view in html should submit all the fields either using the hidden input like in case 1, or we could enforce the behavior with this PR fix (this way should be considered a feature IMO).
- the data comes from an unserialized array, same as case 1 point 2. There are absolutely no problem with this way of submitting data and nothing to change on this side.

So the problem is really about getting the proper array of submitted data when dealing with HTML form's submission.

Wdyt?

backbone87 · 2017-02-24T02:49:34Z

I am against this PR in its current state

Like i already stated here and explained in #20210 (comment) the problem is not the the FormInterface, but the way RequestHandlerInterface prepares the HTTP body to be submitted to the form. The FormInterface isnt responsible to fix shortcomings of the HTTP requests Content-Type. The RequestHandlerInterface was extracted out of the FormInterface in 2.3 exactly for this reason.

The RequestHandlerInterface can traverse the FormInterface tree and decide, if it needs to sanitize specific values, depending on the request (method, content-type, etc) and of course depending on FormConfigInterface (and therefore FormType's options).

fabpot · 2017-03-22T16:57:13Z

To be closed? Updated?

HeahDude · 2017-03-26T18:47:52Z

Closing for now.

ostrolucky · 2019-10-12T22:10:11Z

@HeahDude Any change to continue attempt to fix this? I've taken a look at request handler as suggested in #17771 (comment), but it would require recursive iteration over all form children to find ChoiceType there. It would be quite precedens doing that and I don't think people would be happy to merge something like that

carsonbot added the Status: Needs Review label Feb 11, 2016

javiereguiluz changed the title ~~[WIP] [From] Fix submitting data to ChoiceType with clearMissing false~~ [WIP] [Form] Fix submitting data to ChoiceType with clearMissing false Feb 12, 2016

javiereguiluz added the Form label Feb 13, 2016

HeahDude mentioned this pull request Feb 15, 2016

Update for ChoiceType field via PATCH HTTP method doesn't work #17799

Closed

ostrolucky reviewed Feb 15, 2016
View reviewed changes

HeahDude mentioned this pull request Feb 16, 2016

ChoiceType returns wrong data on submit if clearMissing is false #16802

Closed

HeahDude force-pushed the fix-#16802 branch from ee9dc9b to fa45a70 Compare February 16, 2016 02:09

HeahDude changed the title ~~[WIP] [Form] Fix submitting data to ChoiceType with clearMissing false~~ [Form] Fix submitting data to ChoiceType with clearMissing false Feb 16, 2016

HeahDude changed the title ~~[Form] Fix submitting data to ChoiceType with clearMissing false~~ [Form] Fix submit a ChoiceType with clearMissing false Feb 16, 2016

HeahDude changed the title ~~[Form] Fix submit a ChoiceType with clearMissing false~~ [2.7] [Form] Fix submit a ChoiceType with clearMissing false Feb 17, 2016

HeahDude changed the title ~~[2.7] [Form] Fix submit a ChoiceType with clearMissing false~~ [2.7] [Form] Fix submission of ChoiceType with clearMissing false Feb 17, 2016

fabpot reviewed Feb 17, 2016
View reviewed changes

Silverspur mentioned this pull request Feb 23, 2016

Unchecked 'checkbox' field type not taken into account when using a PATCH request #17899

Open

HeahDude changed the title ~~[2.7] [Form] Fix submission of ChoiceType with clearMissing false~~ [WIP] [2.7] [Form] Fix submission of ChoiceType with clearMissing false Feb 23, 2016

HeahDude changed the title ~~[WIP] [2.7] [Form] Fix submission of ChoiceType with clearMissing false~~ [WIP] [2.7] [Form] Fix submission of nested CheckBox with clearMissing false Feb 23, 2016

HeahDude force-pushed the fix-#16802 branch from f2c8a6f to e071e65 Compare February 23, 2016 12:54

HeahDude mentioned this pull request Feb 29, 2016

[2.7] [Form] fix choice value "false" in ChoiceType #17760

Merged

3 tasks

javiereguiluz added the Bug label Mar 4, 2016

This was referenced Aug 29, 2016

[Form] Consider a violation even if the form is not submitted #18935

Merged

Validation fails for $form->submit($data, true) or SQL errors $form->submit($data, false) #19788

Closed

HeahDude mentioned this pull request Nov 15, 2016

[3.0] [Form] Add/remove collection rows in PATCH requests without sending the existing rows #8301

Closed

HeahDude force-pushed the fix-#16802 branch 2 times, most recently from b1d922a to 096ef21 Compare December 3, 2016 13:24

HeahDude changed the title ~~[WIP] [2.7] [Form] Fix submission of nested CheckBox with clearMissing false~~ [2.7] [Form] Fix submitting checkboxes and radios with PATCH method Dec 3, 2016

HeahDude force-pushed the fix-#16802 branch from 1d465bb to 669e394 Compare December 3, 2016 14:03

HeahDude changed the title ~~[2.7] [Form] Fix submitting checkboxes and radios with PATCH method~~ [Form] Fix submitting checkboxes and radios with PATCH method Dec 3, 2016

This was referenced Dec 3, 2016

[Form] render hidden empty inputs for checkboxes #20210

Closed

Cannot uncheck checkbox field mapped to the entity #20179

Closed

[From] Fix submitting checkboxes and radios with PATCH method

67c577b

HeahDude force-pushed the fix-#16802 branch from 669e394 to 67c577b Compare December 3, 2016 14:37

HeahDude added 2 commits December 3, 2016 16:00

fix tests

7472ba8

fix tests

e3eac28

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 3, 2016

fix tests

e7576a6

nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

ostrolucky reviewed Dec 11, 2016

View reviewed changes

HeahDude closed this Mar 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Fix submitting checkboxes and radios with PATCH method #17771

[Form] Fix submitting checkboxes and radios with PATCH method #17771

HeahDude commented Feb 11, 2016 •

edited

Loading

ostrolucky Feb 15, 2016

HeahDude Feb 15, 2016

fabpot Feb 17, 2016

HeahDude Feb 17, 2016

HeahDude Feb 17, 2016

HeahDude Feb 17, 2016

HeahDude commented Feb 23, 2016

Silverspur commented Feb 23, 2016

HeahDude commented Feb 23, 2016

Silverspur commented Feb 23, 2016

HeahDude commented Feb 23, 2016

Silverspur commented Feb 23, 2016

HeahDude commented Feb 23, 2016

HeahDude commented Feb 24, 2016

HeahDude commented Feb 25, 2016

fabpot commented Jun 14, 2016

HeahDude commented Jun 15, 2016

fabpot commented Jul 1, 2016

HeahDude commented Dec 3, 2016

HeahDude commented Dec 3, 2016

Simperfit commented Dec 9, 2016

ostrolucky Dec 11, 2016

xabbuh commented Dec 18, 2016

HeahDude commented Dec 18, 2016

backbone87 commented Feb 24, 2017

fabpot commented Mar 22, 2017

HeahDude commented Mar 26, 2017

ostrolucky commented Oct 12, 2019

[Form] Fix submitting checkboxes and radios with PATCH method #17771

[Form] Fix submitting checkboxes and radios with PATCH method #17771

Conversation

HeahDude commented Feb 11, 2016 • edited Loading

ostrolucky Feb 15, 2016

Choose a reason for hiding this comment

HeahDude Feb 15, 2016

Choose a reason for hiding this comment

fabpot Feb 17, 2016

Choose a reason for hiding this comment

HeahDude Feb 17, 2016

Choose a reason for hiding this comment

HeahDude Feb 17, 2016

Choose a reason for hiding this comment

HeahDude Feb 17, 2016

Choose a reason for hiding this comment

HeahDude commented Feb 23, 2016

Silverspur commented Feb 23, 2016

HeahDude commented Feb 23, 2016

Silverspur commented Feb 23, 2016

HeahDude commented Feb 23, 2016

Silverspur commented Feb 23, 2016

HeahDude commented Feb 23, 2016

HeahDude commented Feb 24, 2016

HeahDude commented Feb 25, 2016

fabpot commented Jun 14, 2016

HeahDude commented Jun 15, 2016

fabpot commented Jul 1, 2016

HeahDude commented Dec 3, 2016

HeahDude commented Dec 3, 2016

Simperfit commented Dec 9, 2016

ostrolucky Dec 11, 2016

Choose a reason for hiding this comment

xabbuh commented Dec 18, 2016

HeahDude commented Dec 18, 2016

backbone87 commented Feb 24, 2017

fabpot commented Mar 22, 2017

HeahDude commented Mar 26, 2017

ostrolucky commented Oct 12, 2019

HeahDude commented Feb 11, 2016 •

edited

Loading