[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form #22610

emodric · 2017-05-02T10:46:20Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	symfony/symfony-docs#8495

This adds a possibility to use only keyword in form_theme tag to disable usage of globally defined form themes, e.g.

{% form_theme form with ['common.html.twig', 'form/fields.html.twig'] only %}

Otherwise, in order to completely control the rendering of the forms (for example in custom admin interfaces), one would need to use a form theme which has all the possible twig blocks defined to prevent globally defined themes to interfere with the rendering.

only keyword is already used when including a Twig template to transfer only the variables which are explicitly defined in the include tag, so it seemed natural to use it here too.

This, of course, means that the user will need to manually use all of the templates that are required to render the form, including form_div_layout.html.twig

This issue is described in details over at Symfony Demo repo: symfony/demo#515

TODO:

submit changes to the documentation

HeahDude · 2017-07-30T09:34:31Z

src/Symfony/Bridge/Twig/Node/FormThemeNode.php

@@ -16,9 +16,9 @@
 */
 class FormThemeNode extends \Twig_Node
 {
-    public function __construct(\Twig_Node $form, \Twig_Node $resources, $lineno, $tag = null)
+    public function __construct(\Twig_Node $form, \Twig_Node $resources, $only, $lineno, $tag = null)


Should be added as last argument with default false to keep BC.

Isn't this class considered an internal detail?

Actually it's not marked as is, and anyone could extend the parser and the node to use custom implementation.

I don't know if it's a a doable/valid use case, but this API is public for now anyway, so BC should be preserved.

@HeahDude Will do! As I am currently on vacation, give me couple of weeks and I'll do it.

@HeahDude Placed the $only argument at the end and rebased on top of 3.4.

emodric · 2017-09-25T12:16:14Z

Hi, what is the status PR? Do you think it could end up in 3.4?

stof · 2017-09-25T13:35:43Z

src/Symfony/Component/Form/FormRendererEngineInterface.php

     */
-    public function setTheme(FormView $view, $themes);
+    public function setTheme(FormView $view, $themes, $useDefaultThemes = true);


adding the argument in the interface is a BC break. So this cannot be done before 4.0 (and 3.4 should trigger a deprecation warning in case it finds an implementation of the interface without the extra optional attribute)

Okay, makes sense. How should I proceed then?

stof · 2017-09-25T13:35:55Z

src/Symfony/Component/Form/FormRendererInterface.php

     */
-    public function setTheme(FormView $view, $themes);
+    public function setTheme(FormView $view, $themes, $useDefaultThemes = true);


fabpot · 2017-10-01T17:40:05Z

@emodric I like this feature, can you implement the suggestions made by @stof?

emodric · 2017-10-01T17:46:28Z

@fabpot Sure, but how do you want me to handle it?

Open a new PR for deprecations only against 3.4 and target this one for master or did you have something else in mind?

fabpot · 2017-10-01T17:48:13Z

You need to remove the argument and use get_func_args() to determine how it was called to handle the new arg and trigger a dep notice if needed. You can find quite a few examples in the code base doing exactly this.

emodric · 2017-10-01T17:55:37Z

Ah okay, I figured you wanted the feature to go in only in 4.0. I'll do it first thing tomorrow morning.

emodric · 2017-10-02T00:35:18Z

@fabpot I've updated the code to include requested deprecations. I updated the tests too in order not to trigger deprecation notices in them. There is a test failure in Twig bridge tests, but I don't see how it is related to this change.

nicolas-grekas

(failure unrelated)

fabpot · 2017-10-12T21:51:32Z

src/Symfony/Bundle/FrameworkBundle/Templating/Helper/FormHelper.php

-        $this->renderer->setTheme($view, $themes);
+        $args = func_get_args();
+        if (!isset($args[2])) {
+            @trigger_error(sprintf('Calling setTheme method of %s without the third boolean argument $useDefaultThemes is deprecated since 3.4 and will be removed in 4.0. Call the method with the third argument included and use boolean true to keep current behaviour.', self::class), E_USER_DEPRECATED);


I don't understand why we would want to force people to pass a third argument, which should be true by default.

Hm.. I'm not sure what exactly deprecation message should be then if the third argument is optional? Can't we just use it if it's there, and if not, default to true?

I think this is the right approach.

Okay, will fix it ASAP. Where do you want me to document the fact that in 4.0 the interface is going to receive a third argument? I've seen somewhere in the codebase the usage of an inline comment in the style of public function setTheme(FormView $view, $themes /* , $useDefaultThemes = true */ );

Yes, that's what we are doing.

@fabpot Alright, I've updated the code now as per instructions.

nicolas-grekas · 2017-10-13T14:03:35Z

src/Symfony/Component/Form/FormRendererEngineInterface.php

     */
-    public function setTheme(FormView $view, $themes);
+    public function setTheme(FormView $view, $themes /*, $useDefaultThemes = true */);


in all places where this is added, the code should throw a deprecation when the argument is not provided (using func_num_args, see other places in the code base).

@nicolas-grekas As per @fabpot, deprecation notice is not needed if the argument is optional?

I think the current code is good as is :)

…hen rendering a form

fabpot · 2017-10-13T14:51:00Z

Thank you @emodric.

…efault themes when rendering a form (emodric) This PR was merged into the 3.4 branch. Discussion ---------- [Form] [TwigBridge] Added option to disable usage of default themes when rendering a form | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | symfony/symfony-docs#8495 This adds a possibility to use `only` keyword in `form_theme` tag to disable usage of globally defined form themes, e.g. `{% form_theme form with ['common.html.twig', 'form/fields.html.twig'] only %}` Otherwise, in order to completely control the rendering of the forms (for example in custom admin interfaces), one would need to use a form theme which has all the possible twig blocks defined to prevent globally defined themes to interfere with the rendering. `only` keyword is already used when including a Twig template to transfer only the variables which are explicitly defined in the `include` tag, so it seemed natural to use it here too. This, of course, means that the user will need to manually `use` all of the templates that are required to render the form, including `form_div_layout.html.twig` This issue is described in details over at Symfony Demo repo: symfony/demo#515 TODO: - [x] submit changes to the documentation Commits ------- e0681f9 [Form] [TwigBridge] Added option to disable usage of default themes when rendering a form

emodric · 2017-10-13T14:52:19Z

Thanks everyone for review :)

…dric) This PR was merged into the 4.0-dev branch. Discussion ---------- [Form] Add $useDefaultThemes flag to the interfaces | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Followup to #22610 to add `$useDefaultThemes` to the interfaces as discussed in the issue. Commits ------- c22d783 [Form] Add useDefaultThemes flag to the interfaces

… form themes (emodric, javiereguiluz) This PR was merged into the 3.4 branch. Discussion ---------- [Form] Document disabling the usage of globally defined form themes This PR documents the option to disable the usage of globally defined form themes. This is suggested as a feature in symfony/symfony#22610 (reviewed, but not yet merged) Commits ------- 14ef7ae Minor reword 8d45f83 Document disabling the usage of globally defined form themes

carsonbot added Status: Needs Review Form TwigBridge Feature labels May 2, 2017

nicolas-grekas added this to the 3.4 milestone May 2, 2017

emodric mentioned this pull request May 2, 2017

form_row block override in form theme does not work for checkbox symfony/demo#515

Closed

nicolas-grekas changed the base branch from master to 3.4 May 23, 2017 16:57

HeahDude suggested changes Jul 30, 2017

View reviewed changes

emodric force-pushed the form_theme_only branch 2 times, most recently from bdd1ffc to 0d3da35 Compare August 17, 2017 15:11

jvasseur mentioned this pull request Aug 19, 2017

Framework form themes should not be used as default sonata-project/SonataAdminBundle#4611

Closed

emodric force-pushed the form_theme_only branch from 0d3da35 to faa17fe Compare September 25, 2017 12:15

emodric force-pushed the form_theme_only branch 2 times, most recently from 731de67 to 627eeec Compare September 25, 2017 12:17

stof requested changes Sep 25, 2017

View reviewed changes

emodric force-pushed the form_theme_only branch from 627eeec to 9bc309e Compare October 2, 2017 00:24

emodric force-pushed the form_theme_only branch from 9bc309e to 42cd7f1 Compare October 2, 2017 08:52

nicolas-grekas approved these changes Oct 8, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 8, 2017

xabbuh approved these changes Oct 8, 2017

View reviewed changes

emodric mentioned this pull request Oct 9, 2017

[Form] Document disabling the usage of globally defined form themes symfony/symfony-docs#8495

Merged

nicolas-grekas added the Ready label Oct 12, 2017

fabpot requested changes Oct 12, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 12, 2017

emodric force-pushed the form_theme_only branch 3 times, most recently from 3bed88b to 1d89877 Compare October 13, 2017 13:43

nicolas-grekas reviewed Oct 13, 2017

View reviewed changes

[Form] [TwigBridge] Added option to disable usage of default themes w…

e0681f9

…hen rendering a form

emodric force-pushed the form_theme_only branch from 1d89877 to e0681f9 Compare October 13, 2017 14:30

fabpot approved these changes Oct 13, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 13, 2017

fabpot merged commit e0681f9 into symfony:3.4 Oct 13, 2017

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

emodric mentioned this pull request Oct 19, 2017

[Form] Add $useDefaultThemes flag to the interfaces #24628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form #22610

[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form #22610

emodric commented May 2, 2017 •

edited

Loading

HeahDude Jul 30, 2017

sstok Jul 30, 2017 •

edited

Loading

HeahDude Jul 30, 2017

emodric Jul 31, 2017

emodric Aug 17, 2017

emodric commented Sep 25, 2017

stof Sep 25, 2017

emodric Sep 25, 2017

stof Sep 25, 2017

fabpot commented Oct 1, 2017

emodric commented Oct 1, 2017

fabpot commented Oct 1, 2017

emodric commented Oct 1, 2017

emodric commented Oct 2, 2017

nicolas-grekas left a comment

fabpot Oct 12, 2017

emodric Oct 12, 2017

fabpot Oct 12, 2017

emodric Oct 12, 2017 •

edited

Loading

fabpot Oct 13, 2017

emodric Oct 13, 2017

nicolas-grekas Oct 13, 2017

emodric Oct 13, 2017

fabpot Oct 13, 2017

fabpot commented Oct 13, 2017

emodric commented Oct 13, 2017

[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form #22610

[Form] [TwigBridge] Added option to disable usage of default themes when rendering a form #22610

Conversation

emodric commented May 2, 2017 • edited Loading

Choose a reason for hiding this comment

sstok Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emodric commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 1, 2017

emodric commented Oct 1, 2017

fabpot commented Oct 1, 2017

emodric commented Oct 1, 2017

emodric commented Oct 2, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emodric Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 13, 2017

emodric commented Oct 13, 2017

emodric commented May 2, 2017 •

edited

Loading

sstok Jul 30, 2017 •

edited

Loading

emodric Oct 12, 2017 •

edited

Loading