[WIP] [2.7] [Form] fix empty_data option in expanded ChoiceType
#17822
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This one misses its |
Closed
|
@webmozart Can you have a look at this one please ? |
b4858c7 to
d479adf
Compare
|
@symfony/deciders this bug is still present and is very annoying, this PR allows to use Could you please review? |
|
Thank you @HeahDude. |
fabpot
added a commit
that referenced
this pull request
Jun 28, 2016
…oiceType` (HeahDude) This PR was merged into the 2.7 branch. Discussion ---------- [WIP] [2.7] [Form] fix `empty_data` option in expanded `ChoiceType` | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17791 | License | MIT | Doc PR | - It might happen because in `Form::submit()` the handling of `empty_data` [line 597](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Form.php#L597) comes after each child of a compound field has been submitted [line 549](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Form.php#L549). So when `ChoiceType` is `expanded`, `compound` option is defaulted to `true` and it passes its empty submitted data to its children before handling its own `empty_data` option. This PR uses the listener already added in `ChoiceType` only when `expanded` is true to handle `empty_data` at `PRE_SUBMIT` [line 539](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Form.php#L539). - [ ] Fix FQCN in tests for 2.8 - [ ] Remove `choices_as_values` in tests for 3.0 Commits ------- d479adf [Form] fix `empty_data` option in expanded `ChoiceType`
|
Thank you @fabpot |
HeahDude
added a commit
to HeahDude/symfony
that referenced
this pull request
Jun 29, 2016
nicolas-grekas
added a commit
that referenced
this pull request
Jun 29, 2016
This PR was merged into the 3.0 branch. Discussion ---------- [Form] fixed ChoiceTypeTest after #17822 | Q | A | ------------- | --- | Branch? | 3.0 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Commits ------- 777c193 [Form] fixed ChoiceTypeTest after #17822
nicolas-grekas
added a commit
that referenced
this pull request
Jun 29, 2016
* 3.0: [Form] fixed ChoiceTypeTest after #17822
nicolas-grekas
added a commit
that referenced
this pull request
Jun 29, 2016
* 3.1: [Form] fixed ChoiceTypeTest after #17822 [DoctrineBridge] fixed DoctrineChoiceLoaderTest by removing deprecated factory [ci] Upgrade phpunit wrapper deps [FrameworkBundle] Fix fixtures [HttpKernel] Inline ValidateRequestListener logic into HttpKernel fixed HttpKernel dependencies after #18688
This was referenced Jun 30, 2016
Merged
Merged
Merged
Merged
This was referenced Jan 25, 2018
fabpot
added a commit
that referenced
this pull request
Feb 1, 2018
…e (HeahDude) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Fixed empty data on expanded ChoiceType and FileType | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25896 | License | MIT | Doc PR | ~ Alternative of #25924. I don't think we have to do this by adding overhead in master and getting it properly fixed in 2 years. This is bug I've introduced while fixing another bug #17822. Which then has been replicated in #20418 and #24993. I propose instead to clean up the code for all LTS, since this is a wrong behaviour that has never been documented, and most of all unreliable. The `empty_data` can by anything in the view format as long as the reverse view transformation supports it. Even an object derived from the data class could be invokable. I think we should remain consistent with https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/Form.php#L615. Commits ------- 9722c06 [Form] Fixed empty data on expanded ChoiceType and FileType
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It might happen because in
Form::submit()the handling ofempty_dataline 597 comes after each child of a compound field has been submitted line 549.So when
ChoiceTypeisexpanded,compoundoption is defaulted totrueand it passes its empty submitted data to its children before handling its ownempty_dataoption.This PR uses the listener already added in
ChoiceTypeonly whenexpandedis true to handleempty_dataatPRE_SUBMITline 539.choices_as_valuesin tests for 3.0