Skip to content

Twig extensions refatoring to decouple definitions from implementations #20093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Symfony/Bridge/Twig/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
CHANGELOG
=========

3.2.0
-----

* Deprecated the possibility to inject the Form Twig Renderer into the form
extension. Inject it on TwigRendererEngine instead.

2.7.0
-----

Expand Down
95 changes: 32 additions & 63 deletions src/Symfony/Bridge/Twig/Extension/FormExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,26 @@
*/
class FormExtension extends \Twig_Extension implements \Twig_Extension_InitRuntimeInterface
{
/**
* This property is public so that it can be accessed directly from compiled
* templates without having to call a getter, which slightly decreases performance.
*
* @var TwigRendererInterface
*/
public $renderer;
private $renderer;

public function __construct(TwigRendererInterface $renderer)
public function __construct(TwigRendererInterface $renderer = null)
{
if (null !== $this->renderer) {
@trigger_error(sprintf('Passing a Twig Form Renderer to the "%s" constructor is deprecated since version 3.2 and won\'t be possible in 4.0. Pass the Twig_Environment to the TwigRendererEngine constructor instead.', static::class), E_USER_DEPRECATED);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use static::class instead of get_class($this).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

$this->renderer = $renderer;
}

/**
* {@inheritdoc}
*
* To be removed in 4.0
*/
public function initRuntime(\Twig_Environment $environment)
{
$this->renderer->setEnvironment($environment);
if (null !== $this->renderer) {
$this->renderer->setEnvironment($environment);
}
}

/**
Expand Down Expand Up @@ -69,7 +70,7 @@ public function getFunctions()
new \Twig_SimpleFunction('form', null, array('node_class' => 'Symfony\Bridge\Twig\Node\RenderBlockNode', 'is_safe' => array('html'))),
new \Twig_SimpleFunction('form_start', null, array('node_class' => 'Symfony\Bridge\Twig\Node\RenderBlockNode', 'is_safe' => array('html'))),
new \Twig_SimpleFunction('form_end', null, array('node_class' => 'Symfony\Bridge\Twig\Node\RenderBlockNode', 'is_safe' => array('html'))),
new \Twig_SimpleFunction('csrf_token', array($this, 'renderCsrfToken')),
new \Twig_SimpleFunction('csrf_token', array('Symfony\Bridge\Twig\Form\TwigRenderer', 'renderCsrfToken')),
);
}

Expand All @@ -79,7 +80,7 @@ public function getFunctions()
public function getFilters()
{
return array(
new \Twig_SimpleFilter('humanize', array($this, 'humanize')),
new \Twig_SimpleFilter('humanize', array('Symfony\Bridge\Twig\Form\TwigRenderer', 'humanize')),
);
}

Expand All @@ -89,67 +90,35 @@ public function getFilters()
public function getTests()
{
return array(
new \Twig_SimpleTest('selectedchoice', array($this, 'isSelectedChoice')),
new \Twig_SimpleTest('selectedchoice', 'Symfony\Bridge\Twig\Extension\twig_is_selected_choice'),
);
}

/**
* {@inheritdoc}
*/
public function renderCsrfToken($tokenId)
{
return $this->renderer->renderCsrfToken($tokenId);
}

/**
* Makes a technical name human readable.
*
* @param string $text The text to humanize
*
* @return string The humanized text
*/
public function humanize($text)
public function getName()
{
return $this->renderer->humanize($text);
return 'form';
}
}

/**
* Returns whether a choice is selected for a given form value.
*
* 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).
*
* 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
*
* @return bool Whether the choice is selected
*
* @see ChoiceView::isSelected()
*/
public function isSelectedChoice(ChoiceView $choice, $selectedValue)
{
if (is_array($selectedValue)) {
return in_array($choice->value, $selectedValue, true);
}

return $choice->value === $selectedValue;
/**
* Returns whether a choice is selected for a given form value.
*
* This is a function and not callable due to performance reasons.
*
* @param string|array $selectedValue The selected value to compare
*
* @return bool Whether the choice is selected
*
* @see ChoiceView::isSelected()
*/
function twig_is_selected_choice(ChoiceView $choice, $selectedValue)
{
if (is_array($selectedValue)) {
return in_array($choice->value, $selectedValue, true);
}

/**
* {@inheritdoc}
*/
public function getName()
{
return 'form';
}
return $choice->value === $selectedValue;
}
10 changes: 10 additions & 0 deletions src/Symfony/Bridge/Twig/Form/TwigRendererEngine.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ class TwigRendererEngine extends AbstractRendererEngine implements TwigRendererE
*/
private $template;

public function __construct(array $defaultThemes = array(), \Twig_Environment $environment = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we trigger a deprecation when the environment is not injected in the constructor ? It would make sense to remove the setter in 4.0 IMO. The environment is a required dependency, and replacing it after we started using the renderer may break things due to the caching of the block resolution

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{
if (null === $environment) {
@trigger_error(sprintf('Not passing a Twig Environment as the second argument for "%s" constructor is deprecated since version 3.2 and won\'t be possible in 4.0.', static::class), E_USER_DEPRECATED);
}

parent::__construct($defaultThemes);
$this->environment = $environment;
}

/**
* {@inheritdoc}
*/
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bridge/Twig/Form/TwigRendererEngineInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

/**
* @author Bernhard Schussek <bschussek@gmail.com>
*
* @deprecated Deprecated since version 3.2, to be removed in 4.0.
*/
interface TwigRendererEngineInterface extends FormRendererEngineInterface
{
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bridge/Twig/Form/TwigRendererInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

/**
* @author Bernhard Schussek <bschussek@gmail.com>
*
* @deprecated Deprecated since version 3.2, to be removed in 4.0.
*/
interface TwigRendererInterface extends FormRendererInterface
{
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/Twig/Node/FormThemeNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public function compile(\Twig_Compiler $compiler)
{
$compiler
->addDebugInfo($this)
->write('$this->env->getExtension(\'Symfony\Bridge\Twig\Extension\FormExtension\')->renderer->setTheme(')
->write('$this->env->getRuntime(\'Symfony\Bridge\Twig\Form\TwigRenderer\')->setTheme(')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this requires bumping the min requirement for Twig

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? That's already the case (1.26)

->subcompile($this->getNode('form'))
->raw(', ')
->subcompile($this->getNode('resources'))
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/Twig/Node/RenderBlockNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function compile(\Twig_Compiler $compiler)
{
$compiler->addDebugInfo($this);
$arguments = iterator_to_array($this->getNode('arguments'));
$compiler->write('$this->env->getExtension(\'Symfony\Bridge\Twig\Extension\FormExtension\')->renderer->renderBlock(');
$compiler->write('$this->env->getRuntime(\'Symfony\Bridge\Twig\Form\TwigRenderer\')->renderBlock(');

if (isset($arguments[0])) {
$compiler->subcompile($arguments[0]);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/Twig/Node/SearchAndRenderBlockNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class SearchAndRenderBlockNode extends \Twig_Node_Expression_Function
public function compile(\Twig_Compiler $compiler)
{
$compiler->addDebugInfo($this);
$compiler->raw('$this->env->getExtension(\'Symfony\Bridge\Twig\Extension\FormExtension\')->renderer->searchAndRenderBlock(');
$compiler->raw('$this->env->getRuntime(\'Symfony\Bridge\Twig\Form\TwigRenderer\')->searchAndRenderBlock(');

preg_match('/_([^_]+)$/', $this->getAttribute('name'), $matches);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,49 +22,38 @@

class FormExtensionBootstrap3HorizontalLayoutTest extends AbstractBootstrap3HorizontalLayoutTest
{
/**
* @var FormExtension
*/
protected $extension;
use RuntimeLoaderProvider;

protected $testableFeatures = array(
'choice_attr',
);

private $renderer;

protected function setUp()
{
parent::setUp();

$rendererEngine = new TwigRendererEngine(array(
'bootstrap_3_horizontal_layout.html.twig',
'custom_widgets.html.twig',
));
$renderer = new TwigRenderer($rendererEngine, $this->getMock('Symfony\Component\Security\Csrf\CsrfTokenManagerInterface'));

$this->extension = new FormExtension($renderer);

$loader = new StubFilesystemLoader(array(
__DIR__.'/../../Resources/views/Form',
__DIR__.'/Fixtures/templates/form',
));

$environment = new \Twig_Environment($loader, array('strict_variables' => true));
$environment->addExtension(new TranslationExtension(new StubTranslator()));
$environment->addExtension($this->extension);
$environment->addExtension(new FormExtension());

$this->extension->initRuntime($environment);
}

protected function tearDown()
{
parent::tearDown();

$this->extension = null;
$rendererEngine = new TwigRendererEngine(array(
'bootstrap_3_horizontal_layout.html.twig',
'custom_widgets.html.twig',
), $environment);
$this->renderer = new TwigRenderer($rendererEngine, $this->getMock('Symfony\Component\Security\Csrf\CsrfTokenManagerInterface'));
$this->registerTwigRuntimeLoader($environment, $this->renderer);
}

protected function renderForm(FormView $view, array $vars = array())
{
return (string) $this->extension->renderer->renderBlock($view, 'form', $vars);
return (string) $this->renderer->renderBlock($view, 'form', $vars);
}

protected function renderLabel(FormView $view, $label = null, array $vars = array())
Expand All @@ -73,41 +62,41 @@ protected function renderLabel(FormView $view, $label = null, array $vars = arra
$vars += array('label' => $label);
}

return (string) $this->extension->renderer->searchAndRenderBlock($view, 'label', $vars);
return (string) $this->renderer->searchAndRenderBlock($view, 'label', $vars);
}

protected function renderErrors(FormView $view)
{
return (string) $this->extension->renderer->searchAndRenderBlock($view, 'errors');
return (string) $this->renderer->searchAndRenderBlock($view, 'errors');
}

protected function renderWidget(FormView $view, array $vars = array())
{
return (string) $this->extension->renderer->searchAndRenderBlock($view, 'widget', $vars);
return (string) $this->renderer->searchAndRenderBlock($view, 'widget', $vars);
}

protected function renderRow(FormView $view, array $vars = array())
{
return (string) $this->extension->renderer->searchAndRenderBlock($view, 'row', $vars);
return (string) $this->renderer->searchAndRenderBlock($view, 'row', $vars);
}

protected function renderRest(FormView $view, array $vars = array())
{
return (string) $this->extension->renderer->searchAndRenderBlock($view, 'rest', $vars);
return (string) $this->renderer->searchAndRenderBlock($view, 'rest', $vars);
}

protected function renderStart(FormView $view, array $vars = array())
{
return (string) $this->extension->renderer->renderBlock($view, 'form_start', $vars);
return (string) $this->renderer->renderBlock($view, 'form_start', $vars);
}

protected function renderEnd(FormView $view, array $vars = array())
{
return (string) $this->extension->renderer->renderBlock($view, 'form_end', $vars);
return (string) $this->renderer->renderBlock($view, 'form_end', $vars);
}

protected function setTheme(FormView $view, array $themes)
{
$this->extension->renderer->setTheme($view, $themes);
$this->renderer->setTheme($view, $themes);
}
}
Loading