[Form] Made FormView and ChoiceView properties public for performance reasons #5004

webmozart · 2012-07-21T11:27:58Z

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

This PR changes the access to properties of FormView and ChoiceView objects from getters to direct property accesses. On my example form this improves rendering performance for 300ms with PHP templates and 550ms with Twig on my local machine.

Unfortunately, this breaks BC both with 2.0 and with the current master in Form Types and PHP templates. Twig templates are not affected by this change.

2.0:

$formView->set('my_var', 'foobar');
$formView->get('my_var');
$formView->getChild('childName');
$formView['childName'];

master:

$formView->setVar('my_var', 'foobar');
$formView->getVar('my_var');
$formView->get('childName');
$formView['childName'];

this PR:

$formView->vars['my_var'] = 'foobar';
$formView->vars['my_var'];
$formView->children['childName'];
$formView['childName'];

Should we add methods to keep BC with 2.0?

The second part of this PR contains improvements to the rendering of choice fields. These gain another ~500ms for PHP templates and 80ms for Twig. These improvements are BC, unless you overwrote the block "choice_widget_options" in your form themes which then needs to be adapted.

Update:

The PR now includes a BC layer for 2.0.

…to increase performance (PHP +200ms, Twig +150ms)

…, Twig +400ms)

…0ms, Twig +50ms)

…HP +150ms)

…for performance reasons (PHP +100ms)

…ected (PHP +30ms, Twig +30ms)

stof · 2012-07-21T11:33:18Z

src/Symfony/Bridge/Twig/Extension/FormExtension.php

@@ -80,11 +79,45 @@ public function getFunctions()
    public function getFilters()
    {
        return array(
-            'humanize' => new \Twig_Filter_Method($this, 'renderer->humanize'),
+            'humanize'           => new \Twig_Filter_Method($this, 'renderer->humanize'),
+            'is_choice_group'    => new \Twig_Filter_Function('is_array', array('is_safe' => array('html'))),


why marking is_array as being html safe ?

I kept this from the previous code. What is the implication of the "is_safe" setting?

btw, why not using the core iterable test available as of Twig 1.7 ? http://twig.sensiolabs.org/doc/tests/iterable.html
I don't think a choice can be iterable without being a choice group. Am I right ?

is_safe is about disabling the auto-escaping on the output of a filter (if it is used to be displayed), which does not make sense here. So it would be better to remove it (someone looking at an existing twig extension should not see something marked as safe if there is not a good reason to do so as they could start marking unsafe things as safe).
It does not really matters for the form theme itself as it does not display the result but uses it as a test.

I used the iterable test now.

stof · 2012-07-21T11:37:41Z

@bschussek couldn't we keep the getters and setters for BC even if the rendering accesses the public properties directly ?

…E and CHANGELOG

webmozart · 2012-07-21T11:52:33Z

@stof A BC layer for 2.0 is now included. People who upgraded to master already unfortunately need to adapt their code.

stof · 2012-07-21T12:30:54Z

src/Symfony/Bridge/Twig/Extension/FormExtension.php

+    public function getTests()
+    {
+        return array(
+            'choicegroup'    => new \Twig_Test_Function('is_array'),


did you saw my previous comment about iterable ? It would require bumping the Twig requirement to 1.7+ but this is not an issue at all as Twig is fully BC (and already in 1.9)

I did see it later, it seems GH requires page refreshes in order to see added comments. The minimum Twig version for Symfony is 1.8 anyway, so I used iterable now.

inline comments are indeed not refreshed automatically

sstok · 2012-07-21T12:40:57Z

👍

stof · 2012-07-21T13:04:15Z

src/Symfony/Component/Form/CHANGELOG.md

@@ -113,9 +113,9 @@ CHANGELOG
 * deprecated the methods `getDefaultOptions` and `getAllowedOptionValues`
   in FormTypeInterface and FormTypeExtensionInterface
 * options passed during construction can now be accessed from FormConfigInterface
- * added FormBuilderInterface, FormViewInterface and FormConfigEditorInterface
+ * added FormBuilderInterface, FormView and FormConfigEditorInterface


this is wrong. FormView has not been added

@stof

Commits ------- 24b764e [Form] Fixed issues mentioned in the PR 9216816 [Form] Turned Twig filters into tests 310f985 [Form] Added a layer of 2.0 BC methods to FormView and updated UPGRADE and CHANGELOG 5984b18 [Form] Precalculated the closure for deciding whether a choice is selected (PHP +30ms, Twig +30ms) 5dc3c39 [Form] Moved the access to templating helpers out of the choice loop for performance reasons (PHP +100ms) 0ef9acb [Form] Moved the method isChoiceSelected() to the ChoiceView class (PHP +150ms) 8b72766 [Form] Tweaked the generation of option tags for performance (PHP +200ms, Twig +50ms) 400c95b [Form] Replace methods in ChoiceView by public properties (PHP +100ms, Twig +400ms) d072f35 [Form] The properties of FormView are now accessed directly in order to increase performance (PHP +200ms, Twig +150ms) Discussion ---------- [Form] Made FormView and ChoiceView properties public for performance reasons Bug fix: no Feature addition: no Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: - Todo: - This PR changes the access to properties of `FormView` and `ChoiceView` objects from getters to direct property accesses. On [my example form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1) this improves rendering performance for **300ms** with PHP templates and **550ms** with Twig on my local machine. Unfortunately, this breaks BC both with 2.0 and with the current master in Form Types and PHP templates. Twig templates are not affected by this change. 2.0: ``` $formView->set('my_var', 'foobar'); $formView->get('my_var'); $formView->getChild('childName'); $formView['childName']; ``` master: ``` $formView->setVar('my_var', 'foobar'); $formView->getVar('my_var'); $formView->get('childName'); $formView['childName']; ``` this PR: ``` $formView->vars['my_var'] = 'foobar'; $formView->vars['my_var']; $formView->children['childName']; $formView['childName']; ``` Should we add methods to keep BC with 2.0? The second part of this PR contains improvements to the rendering of choice fields. These gain another **~500ms** for PHP templates and **80ms** for Twig. These improvements are BC, unless you overwrote the block "choice_widget_options" in your form themes which then needs to be adapted. **Update:** The PR now includes a BC layer for 2.0. --------------------------------------------------------------------------- by stof at 2012-07-21T11:37:41Z @bschussek couldn't we keep the getters and setters for BC even if the rendering accesses the public properties directly ? --------------------------------------------------------------------------- by bschussek at 2012-07-21T11:52:33Z @stof A BC layer for 2.0 is now included. People who upgraded to master already unfortunately need to adapt their code. --------------------------------------------------------------------------- by sstok at 2012-07-21T12:40:57Z :+1:

Tobion · 2012-07-22T13:55:47Z

I'd suggest to leave FormView::addVars in place as a convenience method. Using array_replace everywhere for modifying the data looks strange to me.

webmozart added 6 commits July 21, 2012 12:56

[Form] The properties of FormView are now accessed directly in order …

d072f35

…to increase performance (PHP +200ms, Twig +150ms)

[Form] Replace methods in ChoiceView by public properties (PHP +100ms…

400c95b

…, Twig +400ms)

[Form] Tweaked the generation of option tags for performance (PHP +20…

8b72766

…0ms, Twig +50ms)

[Form] Moved the method isChoiceSelected() to the ChoiceView class (P…

0ef9acb

…HP +150ms)

[Form] Moved the access to templating helpers out of the choice loop …

5dc3c39

…for performance reasons (PHP +100ms)

[Form] Precalculated the closure for deciding whether a choice is sel…

5984b18

…ected (PHP +30ms, Twig +30ms)

stof reviewed Jul 21, 2012
View reviewed changes

[Form] Added a layer of 2.0 BC methods to FormView and updated UPGRAD…

310f985

…E and CHANGELOG

This was referenced Jul 21, 2012

[Form] The form renderer makes to many deep nested function calls #4962

Closed

[Form] two FormView instances of the same form type influence each other #4987

Closed

stof reviewed Jul 21, 2012
View reviewed changes

webmozart added 2 commits July 21, 2012 17:26

[Form] Turned Twig filters into tests

9216816

[Form] Fixed issues mentioned in the PR

24b764e

fabpot merged commit 24b764e into symfony:master Jul 21, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] Made FormView and ChoiceView properties public for performance reasons #5004

[Form] Made FormView and ChoiceView properties public for performance reasons #5004

Uh oh!

webmozart commented Jul 21, 2012

Uh oh!

stof Jul 21, 2012

Uh oh!

webmozart Jul 21, 2012

Uh oh!

stof Jul 21, 2012

Uh oh!

stof Jul 21, 2012

Uh oh!

webmozart Jul 21, 2012

Uh oh!

stof commented Jul 21, 2012

Uh oh!

webmozart commented Jul 21, 2012

Uh oh!

stof Jul 21, 2012

Uh oh!

webmozart Jul 21, 2012

Uh oh!

stof Jul 21, 2012

Uh oh!

sstok commented Jul 21, 2012

Uh oh!

stof Jul 21, 2012

Uh oh!

webmozart Jul 21, 2012

Uh oh!

Tobion commented Jul 22, 2012

Uh oh!

Uh oh!

Uh oh!

[Form] Made FormView and ChoiceView properties public for performance reasons #5004

[Form] Made FormView and ChoiceView properties public for performance reasons #5004

Uh oh!

Conversation

webmozart commented Jul 21, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Jul 21, 2012

Uh oh!

webmozart commented Jul 21, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstok commented Jul 21, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Jul 22, 2012

Uh oh!

Uh oh!