[Form] Some refactoring of the rendering #1369
Conversation
…callable, introduce FormHelper::renderBlock()
| {% block email_widget %} | ||
| {% spaceless %} | ||
| {% set type = type|default('email') %} | ||
| {{ block('field_widget') }} |
There was a problem hiding this comment.
shouldn't this use form_widget recursively ?
There was a problem hiding this comment.
Yes it could and it was the case before the commit about "optimization".
block can be used here because it would retrieve the correct block and field is always the root type.
When using block you don't have to go through FormExtension::render() which would end up rendering the same block anyway.
There was a problem hiding this comment.
ah right, field is the direct parent of email
|
@vicb On a side note, do you think it would be possible to support form theming in PHP templates too ? Currently, the only way to customize the rendering of forms when using PHP templates is to overwrite the FrameworkBundle's templates, and this impacts all forms. This makes the PHP rendering far less powerful than the Twig one. |
|
@stof I hope to make it possible but I need a little bit more thinking to find the best possible solution which should not look like a hack. |
|
This should not be merged yet, it might have some issue with the variable stack. I am working on it. |
|
Sorted out the issue, it was linked to some local optimization, the code of this PR is ok. |
|
I have pushed a POC of php theming based on this PR to my repo - it is lacking a configuration and cache layer. I have open a thread on the ml to discuss this. |
…the label while rendering a row
|
@fabpot fixed in the last commit. |
Commits ------- 2c1108c [Form] Revert the ability to override anything else than the text of the label while rendering a row da467a6 [Form] Fix the exception message when no block is found while rendering 8670995 [Form] Optimize rendering when the block to render is known 41e07c9 [Form] Optimize rendering ee5d975 [Form] Remove a test which is no more relevant (after recent FileType refactoring) f729c6b [Form] Add the ability to override label & widget options when rendering a row e09ae3f [Form][FrameworkBundle] Make FormHelper::renderSection() recursively callable, introduce FormHelper::renderBlock() e43fb98 [Form][TwigBridge] Make FormExtension::render() recursively callable to ease theming Discussion ---------- [Form] Some refactoring of the rendering # First two commits ## FormExtension::render() can now be called recursively. The main use case is theming support in for collections. Let's consider that you have a collection of `CustomType`, the type hierarchy while rendering the proto would be `field < form < custom < prototype`. Before this change any theme applied to your custom type (i.e. a `custom_row` block) would not have been taken into account while rendering the prototype because of the structure of the `prototype_row` block: ```html {% block prototype_row %} {% spaceless %} <script type="text/html" id="{{ proto_id }}">{{ block('field_row') }}</script> {% endspaceless %} {% endblock prototype_row %} ``` which skip the `custom_row` block rendering to fallback to the `field_row` block rendering. With this PR `prototype_row` recursively calls `FormExtension::render()` ```html {% block prototype_row %} {% spaceless %} <script type="text/html" id="{{ proto_id }}">{{ form_row(form) }}</script> {% endspaceless %} {% endblock prototype_row %} ``` this has for effect to render the block for the parent type (i.e. `custom_row`) ## FormHelper The `FormHelper` has been updated to more closely match the `FormExtension` architecture and the templates have been modified accordingly. `echo $view['form']->renderBlock(<block name>)` is the php equivalent of `{{ block(<block name>) }}`. The attributes are now rendered using a template rather than by the `FormHelper::attributes()` method. Several templates have been fixed. # Third commit The `$varStack` property was used to forward options to the label and the widget when rendering a row. The implementation was not working as expected. The proposed way to override label and widget options is to pass these options in the `label` and `widget` keys while callinf `render_row`. That would be: `{{ form_row(form.field, {"attr": {<row attributes>}, "label" : {"label": <text>, "attr": {<label attr>}}, "widget" : { "attr" : {<widget attributes}} } }}` So there is now the ability to set attributes for the row (`<div>` or `<tr>`). This has been discussed on [the mailing list](http://groups.google.com/group/symfony-devs/browse_thread/thread/17754128ba480545). **I would like to find a compromise with @Seldaek before this gets merged** The `$varStack` property is now only used when recursively calling `FormExtension::render()` # Notes I have preferred to submit several commits in order to ease review and to keep some history. --------------------------------------------------------------------------- by stof at 2011/06/20 05:20:56 -0700 @vicb On a side note, do you think it would be possible to support form theming in PHP templates too ? Currently, the only way to customize the rendering of forms when using PHP templates is to overwrite the FrameworkBundle's templates, and this impacts all forms. This makes the PHP rendering far less powerful than the Twig one. I don't know the Form rendering and the PHPEngine well enough to know if it is feasible for 2.1 or not. --------------------------------------------------------------------------- by vicb at 2011/06/20 05:35:11 -0700 @stof I hope to make it possible but I need a little bit more thinking to find the best possible solution which should not look like a hack. --------------------------------------------------------------------------- by vicb at 2011/06/21 01:13:10 -0700 This should not be merged yet, it might have some issue with the variable stack. I am working on it. --------------------------------------------------------------------------- by vicb at 2011/06/21 01:41:11 -0700 Sorted out the issue, it was linked to some local _optimization_, the code of this PR is ok. --------------------------------------------------------------------------- by vicb at 2011/06/21 02:01:24 -0700 I have pushed a [POC of php theming based on this PR](https://github.com/vicb/symfony/commits/form%2Fphp-theme) to my repo - it is lacking a configuration and cache layer. I have open [a thread on the ml](http://groups.google.com/group/symfony-devs/browse_thread/thread/9b3f131fe116b511) to discuss this. --------------------------------------------------------------------------- by vicb at 2011/06/21 23:40:21 -0700 @fabpot fixed in the last commit.
|
I thought forwarding everything was useful. But the syntax for the simple use case (only label) just became unusable. I'd have liked an auto-handling of the label by doing: |
|
@Seldaek the syntax is the same as it was before this PR. You are proposing an enhancement that you should submit as a separate PR. |
|
Yes I know it didn't change I was referring to your proposal that was an improvement in functionality but not readability. Anyway I'm just asking here to see what @fabpot thinks of it, if it's not gonna happen I'd rather not spend time on it, especially since I don't know the internals of the form rendering too well. |
First two commits
FormExtension::render() can now be called recursively.
The main use case is theming support in for collections. Let's consider that you have a collection of
CustomType, the type hierarchy while rendering the proto would befield < form < custom < prototype. Before this change any theme applied to your custom type (i.e. acustom_rowblock) would not have been taken into account while rendering the prototype because of the structure of theprototype_rowblock:{% block prototype_row %} {% spaceless %} <script type="text/html" id="{{ proto_id }}">{{ block('field_row') }}</script> {% endspaceless %} {% endblock prototype_row %}which skip the
custom_rowblock rendering to fallback to thefield_rowblock rendering.With this PR
prototype_rowrecursively callsFormExtension::render(){% block prototype_row %} {% spaceless %} <script type="text/html" id="{{ proto_id }}">{{ form_row(form) }}</script> {% endspaceless %} {% endblock prototype_row %}this has for effect to render the block for the parent type (i.e.
custom_row)FormHelper
The
FormHelperhas been updated to more closely match theFormExtensionarchitecture and the templates have been modified accordingly.echo $view['form']->renderBlock(<block name>)is the php equivalent of{{ block(<block name>) }}.The attributes are now rendered using a template rather than by the
FormHelper::attributes()method.Several templates have been fixed.
Third commit
The
$varStackproperty was used to forward options to the label and the widget when rendering a row. The implementation was not working as expected. The proposed way to override label and widget options is to pass these options in thelabelandwidgetkeys while callinfrender_row.That would be:
{{ form_row(form.field, {"attr": {<row attributes>}, "label" : {"label": <text>, "attr": {<label attr>}}, "widget" : { "attr" : {<widget attributes}} } }}So there is now the ability to set attributes for the row (
<div>or<tr>).This has been discussed on the mailing list. I would like to find a compromise with @Seldaek before this gets merged
The
$varStackproperty is now only used when recursively callingFormExtension::render()Notes
I have preferred to submit several commits in order to ease review and to keep some history.