Skip to content

[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

Merged
merged 1 commit into from
Sep 29, 2016

Conversation

backbone87
Copy link
Contributor

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

@backbone87 backbone87 changed the title #17580 compound forms without children should be considered rendered implicitly [Form] compound forms without children should be considered rendered implicitly Sep 28, 2016
@fabpot
Copy link
Member

fabpot commented Sep 29, 2016

Thank you @backbone87.

@fabpot fabpot merged commit 8234f2a into symfony:2.7 Sep 29, 2016
fabpot added a commit that referenced this pull request Sep 29, 2016
…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
@fabpot
Copy link
Member

fabpot commented Sep 29, 2016

Actually I merged too fast. Tests do not pass. Can you check that @backbone87? Thanks.
https://travis-ci.org/symfony/symfony/builds/163762900

@fabpot
Copy link
Member

fabpot commented Sep 29, 2016

reverting for now

fabpot added a commit that referenced this pull request Sep 29, 2016
…onsidered rendered implicitly (backbone87)"

This reverts commit eae5a9b, reversing
changes made to fd763ad.
@backbone87
Copy link
Contributor Author

@fabpot @webmozart

the failing test cases build their forms out of plain FormTypes which are compound by default. This fix basically considers every form (sub) tree as rendered that doesnt contain at least one non-compound form type. if not for the CSRF extension adding a non-compound HiddenType, doin {{ form_widget(myFormView) }} or {{ form_row(myFormView) }} when myFormView is a compound type and has only compound type descendants (and CSRF extension disabled) wouldnt output anything. thats because https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormRenderer.php#L158 checks the rendered flag initially for every non label block to make sure every form widget is not rendered more than once.

i could change the test cases to set the leaf FormTypes to compound = false, which would ensure all subtree are rendered, but i question the check at https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormRenderer.php#L158 itself

i have commented out the check and the failing test cases are gone, but one other pops up, i will investigate that tomorrow

fabpot added a commit that referenced this pull request Sep 30, 2016
* 2.7:
  [TwigBundle] Fix CacheWarmingTest are order dependent
  Revert "bug #20080 [Form] compound forms without children should be considered rendered implicitly (backbone87)"
  #17580 compound forms without children should be considered rendered implicitly
@backbone87
Copy link
Contributor Author

backbone87 commented Sep 30, 2016

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

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php#L142

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?

fabpot added a commit that referenced this pull request Oct 1, 2016
* 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
fabpot added a commit that referenced this pull request Oct 1, 2016
* 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
backbone87 added a commit to backbone87/symfony that referenced this pull request Oct 2, 2016
This was referenced Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants