Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Open
filiplikavcan wants to merge13 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromfiliplikavcan:handle-unchecked-checkbox

Conversation

filiplikavcan
Copy link
Contributor

@filiplikavcanfiliplikavcan commentedJan 19, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?yes
New feature?yes
Deprecations?no
TicketsFix#20179,#14938
LicenseMIT
  • write description of this PR

Jibbarth reacted with thumbs up emoji
@carsonbotcarsonbot added this to the4.4 milestoneJan 19, 2022
@carsonbotcarsonbot changed the title[WIP][Form] Missing Data Handling (checkbox edge cases)[Form] [WIP] Missing Data Handling (checkbox edge cases)Jan 19, 2022
@SHTIKOV

This comment was marked as off-topic.

@nicolas-grekas

This comment was marked as outdated.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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?

@nicolas-grekasnicolas-grekas changed the title[Form] [WIP] Missing Data Handling (checkbox edge cases)[Form] Missing Data Handling (checkbox edge cases)Aug 2, 2022
@yceruto
Copy link
Member

Agree for 6.2

@filiplikavcan
Copy link
ContributorAuthor

Tests for php >=8.1 are all green.

@nicolas-grekas
Copy link
Member

Great thanks. OK to rebase+retarget for 6.2?

@filiplikavcanfiliplikavcan changed the base branch from4.4 to6.2August 2, 2022 15:46
@filiplikavcanfiliplikavcanforce-pushed thehandle-unchecked-checkbox branch fromd939f4f toecce817CompareAugust 2, 2022 16:09
nicolas-grekas
nicolas-grekas previously approved these changesAug 2, 2022
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

filiplikavcan reacted with thumbs up emoji
@xabbuh
Copy link
Member

the test failures look related

return $data;
}
}
} catch (\Error $error) {
Copy link
Member

Choose a reason for hiding this comment

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

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

HeahDude reacted with thumbs up emoji
Copy link
ContributorAuthor

@filiplikavcanfiliplikavcanAug 3, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@filiplikavcanfiliplikavcanAug 3, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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

@nicolas-grekasnicolas-grekas dismissed theirstale reviewAugust 3, 2022 08:20

(code is not ready)

@nicolas-grekas
Copy link
Member

Status: needs work

@nicolas-grekas
Copy link
Member

Can you please rebase@filiplikavcan?

filiplikavcan reacted with thumbs up emoji

@mpdude
Copy link
Contributor

mpdude commentedSep 29, 2022•
edited
Loading

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'reGET-requesting a page that contains a checkbox-only form, or if this form has been submitted withmethod="GET" and no checkbox was checked.

Comment on lines 63 to 65
$missingData = $this->missingDataHandler->missingData;

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

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@filiplikavcanfiliplikavcanOct 4, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Done. Is it easier to follow?

$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;
}

@filiplikavcan
Copy link
ContributorAuthor

filiplikavcan commentedOct 4, 2022•
edited
Loading

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'reGET-requesting a page that contains a checkbox-only form, or if this form has been submitted withmethod="GET" and no checkbox was checked.

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

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

@nicolas-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@OskarStarkOskarStarkOskarStark left review comments

@mpdudempdudempdude left review comments

@xabbuhxabbuhxabbuh left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

Cannot uncheck checkbox field mapped to the entity
8 participants
@filiplikavcan@SHTIKOV@nicolas-grekas@yceruto@xabbuh@mpdude@OskarStark@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp