Skip to content

[Form] Make sure to collect child forms created on *_SET_DATA events #33999

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 26, 2019
Merged

[Form] Make sure to collect child forms created on *_SET_DATA events #33999

merged 1 commit into from
Oct 26, 2019

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Oct 16, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #29291
License MIT
Doc PR -

See reproducer provided by @WubbleWobble https://github.com/WubbleWobble/symfony-issue-29291.

@yceruto yceruto added this to the 3.4 milestone Oct 16, 2019
@yceruto yceruto mentioned this pull request Oct 16, 2019
Closed
@stof
Copy link
Member

stof commented Oct 16, 2019

wouldn't this have a bad side-effect if the call to setData happens after creating the form rather than passing the data to the form factory directly ?

@yceruto
Copy link
Member Author

yceruto commented Oct 16, 2019

@stof in that case $this->defaultDataSet is already true and setData() wouldn't be called again.

@yceruto
Copy link
Member Author

yceruto commented Oct 16, 2019 •
edited
Loading

#29291 (comment)
I think this is related to the fact that the fields are added in a PRE_SUBMIT listener. Maybe extractConfiguration is not called properly for dynamic fields, and so the data is incomplete.

I've also checked the case when dynamic child forms are added on PRE_SUBMIT event and they are correctly collected thanks to this code:

symfony/src/Symfony/Component/Form/Extension/DataCollector/FormDataCollector.php

Lines 151 to 155 in 29cabf9

if (!isset($this->dataByForm[$hash])) {
// field was created by form event
$this->collectConfiguration($form);
$this->collectDefaultData($form);
}

@yceruto yceruto force-pushed the form_data_collector_34 branch from 093d50e to f8e779a Compare October 16, 2019 14:19
@yceruto
Copy link
Member Author

yceruto commented Oct 16, 2019 •
edited
Loading

I've changed the solution consistent with collectSubmittedData() approach when the form hash doesn't exist.

@OskarStark
Copy link
Contributor

Is it possible to add a testcase?

@yceruto yceruto force-pushed the form_data_collector_34 branch 2 times, most recently from 703f79e to 3d5f12b Compare October 17, 2019 19:05
@yceruto yceruto force-pushed the form_data_collector_34 branch from 3d5f12b to 50efc1a Compare October 23, 2019 19:50
yceruto added a commit that referenced this pull request Oct 26, 2019
@yceruto
bug #33999 [Form] Make sure to collect child forms created on *_SET_D…
2a5c755 
…ATA events (yceruto)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Make sure to collect child forms created on *_SET_DATA events

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #29291
| License       | MIT
| Doc PR        | -

See reproducer provided by @WubbleWobble https://github.com/WubbleWobble/symfony-issue-29291.

Commits
-------

50efc1a Make sure to collect child forms created on *_SET_DATA events
@yceruto yceruto merged commit 50efc1a into symfony:3.4 Oct 26, 2019
@yceruto yceruto deleted the form_data_collector_34 branch October 26, 2019 10:33
This was referenced Nov 1, 2019
Merged
Merged
@dependabot dependabot bot mentioned this pull request Mar 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug Form Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@yceruto @stof @OskarStark @xabbuh @carsonbot