Skip to content

Commit

Permalink
[Form] Precalculated the closure for deciding whether a choice is sel…
Browse files Browse the repository at this point in the history
…ected (PHP +30ms, Twig +30ms)
  • Loading branch information
webmozart committed Jul 21, 2012
1 parent 5dc3c39 commit 5984b18
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 82 deletions.
22 changes: 16 additions & 6 deletions src/Symfony/Bridge/Twig/Extension/FormExtension.php
Expand Up @@ -88,12 +88,18 @@ public function getFilters()
/**
* Returns whether a choice is selected for a given form value.
*
* This method exists for the sole purpose that Twig performs (a lot) better
* with filters than with methods of an object.
* Unfortunately Twig does not support an efficient way to execute the
* "is_selected" closure passed to the template by ChoiceType. It is faster
* to implement the logic here (around 65ms for a specific form).
*
* To give this some perspective, I'm currently testing this on a form with
* a large list of entity fields. Using the filter is around 220ms faster than
* accessing the method directly on the object in the Twig template.
* Directly implementing the logic here is also faster than doing so in
* ChoiceView (around 30ms).
*
* The worst option tested so far is to implement the logic in ChoiceView
* and access the ChoiceView method directly in the template. Doing so is
* around 220ms slower than doing the method call here in the filter. Twig
* seems to be much more efficient at executing filters than at executing
* methods of an object.
*
* @param ChoiceView $choice The choice to check.
* @param string|array $selectedValue The selected value to compare.
Expand All @@ -104,7 +110,11 @@ public function getFilters()
*/
public function isChoiceSelected(ChoiceView $choice, $selectedValue)
{
return $choice->isSelected($selectedValue);
if (is_array($selectedValue)) {
return false !== array_search($choice->value, $selectedValue, true);
}

return $choice->value === $selectedValue;
}

/**
Expand Down
Expand Up @@ -18,6 +18,7 @@
use Symfony\Bridge\Twig\Tests\Extension\Fixtures\StubTranslator;
use Symfony\Bridge\Twig\Tests\Extension\Fixtures\StubFilesystemLoader;
use Symfony\Component\Form\FormView;
use Symfony\Component\Form\Extension\Core\View\ChoiceView;
use Symfony\Component\Form\Tests\AbstractDivLayoutTest;

class FormExtensionDivLayoutTest extends AbstractDivLayoutTest
Expand Down Expand Up @@ -105,6 +106,39 @@ public function testThemeBlockInheritanceUsingExtend()
);
}

public function isChoiceSelectedProvider()
{
// The commented cases should not be necessary anymore, because the
// choice lists should assure that both values passed here are always
// strings
return array(
// array(true, 0, 0),
array(true, '0', '0'),
array(true, '1', '1'),
// array(true, false, 0),
// array(true, true, 1),
array(true, '', ''),
// array(true, null, ''),
array(true, '1.23', '1.23'),
array(true, 'foo', 'foo'),
array(true, 'foo10', 'foo10'),
array(true, 'foo', array(1, 'foo', 'foo10')),

array(false, 10, array(1, 'foo', 'foo10')),
array(false, 0, array(1, 'foo', 'foo10')),
);
}

/**
* @dataProvider isChoiceSelectedProvider
*/
public function testIsChoiceSelected($expected, $choice, $value)
{
$choice = new ChoiceView($choice, $choice . ' label');

$this->assertSame($expected, $this->extension->isChoiceSelected($choice, $value));
}

protected function renderEnctype(FormView $view)
{
return (string) $this->extension->renderer->renderEnctype($view);
Expand Down
Expand Up @@ -6,6 +6,6 @@
<?php echo $formHelper->block('choice_widget_options', array('choices' => $choice)) ?>
</optgroup>
<?php else: ?>
<option value="<?php echo $view->escape($choice->value) ?>"<?php if ($choice->isSelected($value)): ?> selected="selected"<?php endif?>><?php echo $view->escape($translatorHelper->trans($choice->label, array(), $translation_domain)) ?></option>
<option value="<?php echo $view->escape($choice->value) ?>"<?php if ($is_selected($choice->value, $value)): ?> selected="selected"<?php endif?>><?php echo $view->escape($translatorHelper->trans($choice->label, array(), $translation_domain)) ?></option>
<?php endif ?>
<?php endforeach ?>
14 changes: 14 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
Expand Up @@ -90,6 +90,20 @@ public function buildView(FormView $view, FormInterface $form, array $options)
'empty_value' => null,
));

// The decision, whether a choice is selected, is potentially done
// thousand of times during the rendering of a template. Provide a
// closure here that is optimized for the value of the form, to
// avoid making the type check inside the closure.
if ($options['multiple']) {
$view->vars['is_selected'] = function ($choice, array $values) {
return false !== array_search($choice, $values, true);
};
} else {
$view->vars['is_selected'] = function ($choice, $value) {
return $choice === $value;
};
}

// Check if the choices already contain the empty value
// Only add the empty value option if this is not the case
if (0 === count($options['choice_list']->getIndicesForValues(array('')))) {
Expand Down
75 changes: 0 additions & 75 deletions src/Symfony/Component/Form/Tests/FormRendererTest.php

This file was deleted.

0 comments on commit 5984b18

Please sign in to comment.