merged branch bschussek/performance (PR #4909)

Commits ------- 69e5e58 [Form] Individual rows of CollectionType cannot be styled anymore for performance reasons Discussion ---------- [Form] Individual rows of CollectionType cannot be styled anymore Bug fix: no Feature addition: no Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: #2806, #4733 Todo: - Individual theming of blocks in a row of a collection form is now unsupported: ``` {% block _author_tags_0_label %} {# ... #} {% endblock %} {% block _author_tags_1_label %} {# ... #} {% endblock %} ``` Instead, it is now possible to define a generic template for all rows, where the word "entry" replaces the previous occurence of the row index: ``` {% block _author_tags_entry_label %} {# ... #} {% endblock %} ``` The main motivation for this change is performance. Looking up whether individual styles exist for each single block within each row costs a lot of time. Because the row index is included in the block names, caching is virtually impossible. For [this specific, heavy form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1), this PR decreases rendering time from **7.7** to **2.5 seconds** on my machine. --------------------------------------------------------------------------- by fabpot at 2012-07-14T10:55:29Z @bschussek Can you also create a PR on symfony docs to update the documentation?
2012-07-14 13:03:43 +02:00 · 2012-07-14 13:03:43 +02:00 · 881aebeecb
parent cf41bf8776 69e5e58629
commit 881aebeecb
10 changed files with 118 additions and 16 deletions
--- a/UPGRADE-2.1.md
+++ b/UPGRADE-2.1.md
@ -576,6 +576,30 @@
    {% endblock %}
    ````

+  * Custom styling of individual rows of a collection form has been removed for
+    performance reasons. Instead, all rows now have the same block name, where
+    the word "entry" replaces the previous occurence of the row index.
+
+    Before:
+
+    ```
+    {% block _author_tags_0_label %}
+        {# ... #}
+    {% endblock %}
+
+    {% block _author_tags_1_label %}
+        {# ... #}
+    {% endblock %}
+    ```
+
+    After:
+
+    ```
+    {% block _author_tags_entry_label %}
+        {# ... #}
+    {% endblock %}
+    ```
+
 #### Other BC Breaks

  * The order of the first two arguments of the methods `createNamed` and
--- a/src/Symfony/Bridge/Twig/Extension/FormExtension.php
+++ b/src/Symfony/Bridge/Twig/Extension/FormExtension.php
@ -245,7 +245,7 @@ class FormExtension extends \Twig_Extension
            $this->varStack[$rendering]['variables'] = array_replace_recursive($this->varStack[$rendering]['variables'], $variables);
        } else {
            $types = $view->getVar('types');
-            $types[] = $custom;
+            $types[] = $view->getVar('full_block_name');
            $typeIndex = count($types) - 1;
            $this->varStack[$rendering] = array(
                'variables' => array_replace_recursive($view->getVars(), $variables),
--- a/src/Symfony/Bridge/Twig/Tests/Extension/custom_widgets.html.twig
+++ b/src/Symfony/Bridge/Twig/Tests/Extension/custom_widgets.html.twig
@ -1,5 +1,16 @@
 {% block _text_id_widget %}
-<div id="container">
-    {{ form_widget(form) }}
-</div>
+{% spaceless %}
+    <div id="container">
+        {{ form_widget(form) }}
+    </div>
+{% endspaceless %}
 {% endblock _text_id_widget %}
+
+{% block _name_entry_label %}
+{% spaceless %}
+    {% if label is empty %}
+        {% set label = name|humanize %}
+    {% endif %}
+    <label>Custom label: {{ label|trans({}, translation_domain) }}</label>
+{% endspaceless %}
+{% endblock _name_entry_label %}
--- a/src/Symfony/Bundle/FrameworkBundle/Templating/Helper/FormHelper.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Templating/Helper/FormHelper.php
@ -246,7 +246,7 @@ class FormHelper extends Helper
            $variables = array_replace_recursive($this->varStack[$rendering]['variables'], $variables);
        } else {
            $types = $view->getVar('types');
-            $types[] = $custom;
+            $types[] = $view->getVar('full_block_name');
            $typeIndex = count($types) - 1;
            $variables = array_replace_recursive($view->getVars(), $variables);
            $this->varStack[$rendering]['types'] = $types;
--- a/src/Symfony/Bundle/FrameworkBundle/Tests/Templating/Helper/Resources/Custom/_name_entry_label.html.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Tests/Templating/Helper/Resources/Custom/_name_entry_label.html.php
@ -0,0 +1,2 @@
+<?php if (!$label) { $label = $view['form']->humanize($name); } ?>
+<label>Custom label: <?php echo $view->escape($view['translator']->trans($label, array(), $translation_domain)) ?></label>
--- a/src/Symfony/Component/Form/CHANGELOG.md
+++ b/src/Symfony/Component/Form/CHANGELOG.md
@ -170,3 +170,5 @@ CHANGELOG
   * `getExtensions`
   * `setExtensions`
 * ChoiceType now caches its created choice lists to improve performance
+ * [BC BREAK] Rows of a collection field cannot be themed individually anymore. All rows in the collection
+   field now have the same block names, which contains "entry" where it previously contained the row index.
--- a/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php
+++ b/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php
@ -16,6 +16,7 @@ use Symfony\Component\Form\FormBuilderInterface;
 use Symfony\Component\Form\FormViewInterface;
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\Form\Extension\Core\EventListener\ResizeFormListener;
+use Symfony\Component\OptionsResolver\Options;
 use Symfony\Component\OptionsResolver\OptionsResolverInterface;

 class CollectionType extends AbstractType
@ -73,6 +74,12 @@ class CollectionType extends AbstractType
     */
    public function setDefaultOptions(OptionsResolverInterface $resolver)
    {
+        $optionsFilter = function (Options $options, $value) {
+            $value['block_name'] = 'entry';
+
+            return $value;
+        };
+
        $resolver->setDefaults(array(
            'allow_add'      => false,
            'allow_delete'   => false,
@ -81,6 +88,10 @@ class CollectionType extends AbstractType
            'type'           => 'text',
            'options'        => array(),
        ));
+
+        $resolver->setFilters(array(
+            'options' => $optionsFilter,
+        ));
    }

    /**
--- a/src/Symfony/Component/Form/Extension/Core/Type/FormType.php
+++ b/src/Symfony/Component/Form/Extension/Core/Type/FormType.php
@ -59,6 +59,7 @@ class FormType extends AbstractType
    public function buildView(FormViewInterface $view, FormInterface $form, array $options)
    {
        $name = $form->getName();
+        $blockName = $options['block_name'] ?: $form->getName();
        $readOnly = $options['read_only'];
        $translationDomain = $options['translation_domain'];

@ -67,23 +68,30 @@ class FormType extends AbstractType
                throw new FormException('Form node with empty name can be used only as root form node.');
            }

-            if ('' !== ($parentFullName = $view->getParent()->getVar('full_name'))) {
-                $id = sprintf('%s_%s', $view->getParent()->getVar('id'), $name);
+            $parentView = $view->getParent();
+
+            if ('' !== ($parentFullName = $parentView->getVar('full_name'))) {
+                $id = sprintf('%s_%s', $parentView->getVar('id'), $name);
                $fullName = sprintf('%s[%s]', $parentFullName, $name);
+                $fullBlockName = sprintf('%s_%s', $parentView->getVar('full_block_name'), $blockName);
            } else {
                $id = $name;
                $fullName = $name;
+                $fullBlockName = '_' . $blockName;
            }

-            // Complex fields are read-only if themselves or their parent is.
-            $readOnly = $readOnly || $view->getParent()->getVar('read_only');
+            // Complex fields are read-only if they themselves or their parents are.
+            if (!$readOnly) {
+                $readOnly = $parentView->getVar('read_only');
+            }

            if (!$translationDomain) {
-                $translationDomain = $view->getParent()->getVar('translation_domain');
+                $translationDomain = $parentView->getVar('translation_domain');
            }
        } else {
            $id = $name;
            $fullName = $name;
+            $fullBlockName = '_' . $blockName;

            // Strip leading underscores and digits. These are allowed in
            // form names, but not in HTML4 ID attributes.
@ -105,6 +113,7 @@ class FormType extends AbstractType
            'id'                 => $id,
            'name'               => $name,
            'full_name'          => $fullName,
+            'full_block_name'    => $fullBlockName,
            'read_only'          => $readOnly,
            'errors'             => $form->getErrors(),
            'valid'              => $form->isBound() ? $form->isValid() : true,
@ -177,7 +186,14 @@ class FormType extends AbstractType
            return false !== $options['property_path'];
        };

+        // If data is given, the form is locked to that data
+        // (independent of its value)
+        $resolver->setOptional(array(
+            'data',
+        ));
+
        $resolver->setDefaults(array(
+            'block_name'         => null,
            'data_class'         => $dataClass,
            'empty_data'         => $emptyData,
            'trim'               => true,
@ -198,12 +214,6 @@ class FormType extends AbstractType
            'translation_domain' => null,
        ));

-        // If data is given, the form is locked to that data
-        // (independent of its value)
-        $resolver->setOptional(array(
-            'data',
-        ));
-
        $resolver->setAllowedTypes(array(
            'attr'       => 'array',
            'label_attr' => 'array',
--- a/src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php
+++ b/src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php
@ -557,6 +557,27 @@ abstract class AbstractDivLayoutTest extends AbstractLayoutTest
            ]
        /following-sibling::input[@type="hidden"]
    ]
+'
+        );
+    }
+
+    /**
+     * The block "_name_child_label" should be overridden in the theme of the
+     * implemented driver.
+     */
+    public function testCollectionRowWithCustomBlock()
+    {
+        $collection = array('one', 'two', 'three');
+        $form = $this->factory->createNamedBuilder('name', 'collection', $collection)
+            ->getForm();
+
+        $this->assertWidgetMatchesXpath($form->createView(), array(),
+'/div
+    [
+        ./div[./label[.="Custom label: [trans]0[/trans]"]]
+        /following-sibling::div[./label[.="Custom label: [trans]1[/trans]"]]
+        /following-sibling::div[./label[.="Custom label: [trans]2[/trans]"]]
+    ]
 '
        );
    }
--- a/src/Symfony/Component/Form/Tests/AbstractTableLayoutTest.php
+++ b/src/Symfony/Component/Form/Tests/AbstractTableLayoutTest.php
@ -338,6 +338,27 @@ abstract class AbstractTableLayoutTest extends AbstractLayoutTest
            ]
    ]
    [count(.//input)=3]
+'
+        );
+    }
+
+    /**
+     * The block "_name_child_label" should be overridden in the theme of the
+     * implemented driver.
+     */
+    public function testCollectionRowWithCustomBlock()
+    {
+        $collection = array('one', 'two', 'three');
+        $form = $this->factory->createNamedBuilder('name', 'collection', $collection)
+            ->getForm();
+
+        $this->assertWidgetMatchesXpath($form->createView(), array(),
+'/table
+    [
+        ./tr[./td/label[.="Custom label: [trans]0[/trans]"]]
+        /following-sibling::tr[./td/label[.="Custom label: [trans]1[/trans]"]]
+        /following-sibling::tr[./td/label[.="Custom label: [trans]2[/trans]"]]
+    ]
 '
        );
    }