-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] compound forms without children should be considered rendered implicitly #20080
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
backbone87
commented
Sep 28, 2016
Q | A |
---|---|
Branch? | 2.7 |
Bug fix? | yes |
New feature? | no |
BC breaks? | possibly |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #17580 |
License | MIT |
Doc PR | n/a |
e734222
to
234efa9
Compare
…ndered implicitly
234efa9
to
8234f2a
Compare
Thank you @backbone87. |
…d rendered implicitly (backbone87) This PR was merged into the 2.7 branch. Discussion ---------- [Form] compound forms without children should be considered rendered implicitly | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | possibly | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17580 | License | MIT | Doc PR | n/a Commits ------- 8234f2a #17580 compound forms without children should be considered rendered implicitly
Actually I merged too fast. Tests do not pass. Can you check that @backbone87? Thanks. |
reverting for now |
the failing test cases build their forms out of plain i could change the test cases to set the leaf i have commented out the check and the failing test cases are gone, but one other pops up, i will investigate that tomorrow |
The following test case is failing, if i remove the check at https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormRenderer.php#L158 This is because form_rest relies on the check in the FormRenderer. Although form_rest does only output not already rendered child forms, it only makes the check explicitly for children ( https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L319 ) and then starts the default renderering process for unrendered childs , relying on FormRenderer to check for already rendered descendants within these childs. You could completely remove the check at https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L319 and you will get exactly the same result, because form_rest just calls form_row on children, which itself skips already rendered children. imo the check should be done consistently only in one place. either in the form theme or in the form renderer. however we can only move it to the renderer, because moving it (completely) to the form theme would require modifying them, which could be a hugh break in BC. but i think it would be actually better to do it in the form theme, since this allows you do output form rows/widgets multiple times (for example to populate a JS template fragment). edit: so for this issue, i think just making sure that there are non-compound leaf forms, should be enough, but idk if this assumption is in line with the intented use of the form component. @fabpot should i open a new PR, or you will "remerge" this one? |
* 2.8: [TwigBundle] added missing dependencies for tests fixed CS [TwigBundle] Adjust CacheWarmingTest for TemplateCacheWarmer introduced in 2.8 [TwigBundle] Fix CacheWarmingTest are order dependent Revert "bug #20080 [Form] compound forms without children should be considered rendered implicitly (backbone87)" Fix #19943 Make sure to process each interface metadata only once #17580 compound forms without children should be considered rendered implicitly
* 3.1: [TwigBundle] added missing dependencies for tests fixed CS adding missing dep [TwigBundle] Adjust CacheWarmingTest for TemplateCacheWarmer introduced in 2.8 [TwigBundle] Fix CacheWarmingTest are order dependent Revert "bug #20080 [Form] compound forms without children should be considered rendered implicitly (backbone87)" [2.7][VarDumper] Fix PHP 7.1 compat [2.8][VarDumper] Fix PHP 7.1 compat silent file operation to avoid open basedir issues Fix #19943 Make sure to process each interface metadata only once #17580 compound forms without children should be considered rendered implicitly
…nsidered rendered