[Form] Missing Data Handling (checkbox edge cases) #45081

filiplikavcan · 2022-01-19T18:04:52Z

Q	A
Branch?	6.2
Bug fix?	yes
New feature?	yes
Deprecations?	no
Tickets	Fix #20179, #14938
License	MIT

write description of this PR

nicolas-grekas

Sorry for the delay reviewing this, it's definitely interesting!

I force-pushed on your fork to turn the trait into a class. Please fetch + reset --hard before reopening the branch. There are some tests failing that need to be addressed.

I'm just wondering if this shouldn't go on 6.2: yes, this can be seen as a bug fix, but it's also a new behavior to me.

/cc @symfony/mergers WDYT?

yceruto · 2022-08-02T13:42:10Z

Agree for 6.2

filiplikavcan · 2022-08-02T15:34:49Z

Tests for php >=8.1 are all green.

nicolas-grekas · 2022-08-02T15:36:53Z

Great thanks. OK to rebase+retarget for 6.2?

nicolas-grekas

LGTM thanks!

xabbuh · 2022-08-02T17:09:40Z

the test failures look related

.gitignore

xabbuh · 2022-08-03T07:13:20Z

src/Symfony/Component/Form/MissingDataHandler.php

+                    return $data;
+                }
+            }
+        } catch (\Error $error) {


Catching type errors here doesn't look like the right solution to me. We should investigate instead why the $type property in FormConfigBuilder isn't initialized at this point and fix that root issue instead.

I don't like it either.

Another solution could be to make FormConfigBuilderInterface::getType() nullable. Then null state would have to be handled in these methods where getType() is called:

ValidatorDataCollector#getCasters
FormDataExtractor#extractConfiguration
FormDataCollector#getCasters
FormTypeCsrfExtension#finishView
FormTypeCsrfExtension#buildForm
BaseType#buildView
Button#createView

I don't know if null/undefined type is a valid case or not.

I can look into why we run into this error here. But that will probably not happen before the weekend.

Thank you. When I created this PR on Jan 19 it worked. These commits look related: a53b450d 488a262a

(code is not ready)

nicolas-grekas · 2022-08-30T16:06:59Z

Status: needs work

nicolas-grekas · 2022-09-28T12:35:13Z

Can you please rebase @filiplikavcan?

src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php

mpdude · 2022-09-29T21:09:27Z

A form that consists of unchecked checkboxes only will not be recognized as being submitted with the standard $form->handleRequest(...); if ($form->isSubmitted() ...) { ... } pattern. Does this PR intend to address this, or does it cover other edge cases I am not aware of?

If yes, you might want to mention #20210 in the list of tickets as well.

Update: I have been looking at this code for over 10 minutes now and I am unable to figure out how it would be able to tell whether we're GET-requesting a page that contains a checkbox-only form, or if this form has been submitted with method="GET" and no checkbox was checked.

mpdude · 2022-09-29T21:20:49Z

src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php

+                $missingData = $this->missingDataHandler->missingData;
+
+                if ($missingData === $data = $this->missingDataHandler->handle($form, $request->query->all()[$name] ?? $missingData)) {


I cannot follow this code and would love if we could break this down into smaller chunks, maybe using expressive variable names.

Done. Is it easier to follow?

symfony/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php

Lines 60 to 69 in 4dcd5a4

$missingData = $this->missingDataHandler->missingData;

$queryData = $request->query->all()[$name] ?? $missingData;

$data = $this->missingDataHandler->handle($form, $queryData);

if ($missingData === $data) {

// Don't submit GET requests if the form's name does not exist

// in the request

return;

}

…equestHandler when handling GET requests)

filiplikavcan · 2022-10-04T18:11:42Z

Update: I have been looking at this code for over 10 minutes now and I am unable to figure out how it would be able to tell whether we're GET-requesting a page that contains a checkbox-only form, or if this form has been submitted with method="GET" and no checkbox was checked.

I think it would not be possible to tell the two situations apart. Look at this test:

symfony/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php

Lines 134 to 145 in 4dcd5a4

    
           public function testSubmitSimpleCheckboxFormWithEmptyData($method) 
        
           { 
        
               $form = $this->factory->createNamed('checkbox', CheckboxType::class, true, [ 
        
                   'method' => $method, 
        
               ]); 
        
               $this->setRequestData($method, []); 
        
               $this->requestHandler->handleRequest($form, $this->request); 
        
               $this->assertFalse($form->getData()); 
        
           }

filiplikavcan requested review from xabbuh and yceruto as code owners January 19, 2022 18:04

carsonbot added Status: Needs Work Status: Needs Review Bug Form and removed Status: Needs Work labels Jan 19, 2022

carsonbot added this to the 4.4 milestone Jan 19, 2022

carsonbot changed the title ~~[WIP][Form] Missing Data Handling (checkbox edge cases)~~ [Form] [WIP] Missing Data Handling (checkbox edge cases) Jan 19, 2022

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

nicolas-grekas force-pushed the handle-unchecked-checkbox branch from bb97fdb to adb9016 Compare August 2, 2022 10:42

nicolas-grekas reviewed Aug 2, 2022

View reviewed changes

nicolas-grekas changed the title ~~[Form] [WIP] Missing Data Handling (checkbox edge cases)~~ [Form] Missing Data Handling (checkbox edge cases) Aug 2, 2022

filiplikavcan changed the base branch from 4.4 to 6.2 August 2, 2022 15:46

filiplikavcan force-pushed the handle-unchecked-checkbox branch from d939f4f to ecce817 Compare August 2, 2022 16:09

nicolas-grekas previously approved these changes Aug 2, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 2, 2022

xabbuh reviewed Aug 3, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Aug 30, 2022

nicolas-grekas modified the milestones: 4.4, 6.2 Sep 28, 2022

OskarStark reviewed Sep 28, 2022

View reviewed changes

src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php Outdated Show resolved Hide resolved

mpdude reviewed Sep 29, 2022

View reviewed changes

filiplikavcan added 13 commits October 4, 2022 19:46

[Form] Missing Data Handling (checkbox)

d47a57f

Fix failing tests (missingDataHandler was not used in HttpFoundationR…

753382d

…equestHandler when handling GET requests)

Remove unnecessary 'use'

9da6d1c

Force push

82535ab

Force push

4eb6dad

Add original logic (has/all) back

663984e

CS

591b4b9

CS

d1853fd

Handle uninitialized FormConfigBuilder::$type

5a5fe0a

CS

474ac30

Force restart tests

ab3c101

[WIP] Rebase to 6.2

5ef1388

[WIP] Code clarifications

4dcd5a4

filiplikavcan force-pushed the handle-unchecked-checkbox branch from 5aad0a4 to 4dcd5a4 Compare October 4, 2022 17:50

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

		$missingData = $this->missingDataHandler->missingData;

		if ($missingData === $data = $this->missingDataHandler->handle($form, $request->query->all()[$name] ?? $missingData)) {

	$missingData = $this->missingDataHandler->missingData;
	$queryData = $request->query->all()[$name] ?? $missingData;

	$data = $this->missingDataHandler->handle($form, $queryData);

	if ($missingData === $data) {
	// Don't submit GET requests if the form's name does not exist
	// in the request
	return;
	}

Uh oh!

[Form] Missing Data Handling (checkbox edge cases) #45081

Are you sure you want to change the base?

[Form] Missing Data Handling (checkbox edge cases) #45081

Conversation

filiplikavcan commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as outdated.

nicolas-grekas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto commented Aug 2, 2022

Uh oh!

filiplikavcan commented Aug 2, 2022

Uh oh!

nicolas-grekas commented Aug 2, 2022

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Aug 2, 2022

Uh oh!

Uh oh!

xabbuh Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

filiplikavcan Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

filiplikavcan Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Aug 30, 2022

Uh oh!

nicolas-grekas commented Sep 28, 2022

Uh oh!

Uh oh!

mpdude commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpdude Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

filiplikavcan Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiplikavcan commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

filiplikavcan commented Jan 19, 2022 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

filiplikavcan Aug 3, 2022 •

edited

Loading

filiplikavcan Aug 3, 2022 •

edited

Loading

mpdude commented Sep 29, 2022 •

edited

Loading

filiplikavcan Oct 4, 2022 •

edited

Loading

filiplikavcan commented Oct 4, 2022 •

edited

Loading