Twig extensions refatoring to decouple definitions from implementations #20093

fabpot · 2016-09-29T18:39:33Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	see twigphp/Twig#1913
License	MIT
Doc PR	not yet

This PR tries to use the new Twig runtime loader to the Twig bridge extensions.

linaori · 2016-09-29T19:42:45Z

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

    {
+        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.', get_class($this)), E_USER_DEPRECATED);
+        }


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

linaori · 2016-09-29T19:50:44Z

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

+ *
+ * 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).


It is faster to implement the logic as function (around 65ms for a specific form).
as function explains a lot more at once, took me a bit to actually grasp the relation between this function and getTests without

This part is not new, just moved in the file. I would say that we could removed it entirely, WDYT?

It does give a nice explanation of why it's chosen to be a function instead of closure/class object property. If you're referring to the 3 paragraphs, I don't think they add a real value to the docblock, but a small This is a function and not callable due to performance reasons. would be good to prevent people from making PRs to refactor it again.

linaori · 2016-09-29T19:55:18Z

I have the feeling #20092 serves as a base for this PR? I noticed duplicate changes

fabpot · 2016-09-29T19:55:59Z

Yes, I will rebase this one when #20092 is merged (probably in the next hour or so if I don't have any other comments).

fabpot · 2016-09-29T21:13:56Z

Rebased now.

stof · 2016-09-30T09:42:06Z

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

-     *
-     * @see ChoiceView::isSelected()
-     */
-    public function isSelectedChoice(ChoiceView $choice, $selectedValue)


to be more BC (in case someone calls it directly), we could make it a static method rather than a function

This is clearly not an extension point, and as performance for this one is critical (the reason why we have this in the first place), I prefer to keep the function.

stof · 2016-09-30T09:44:29Z

src/Symfony/Bridge/Twig/Form/TwigRendererEngine.php

@@ -29,6 +29,12 @@ class TwigRendererEngine extends AbstractRendererEngine implements TwigRendererE
     */
    private $template;

+    public function __construct(array $defaultThemes = array(), \Twig_Environment $environment = null)


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

stof · 2016-09-30T09:45:50Z

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

@@ -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(')


this requires bumping the min requirement for Twig

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

…m implementations (fabpot) This PR was merged into the 3.2-dev branch. Discussion ---------- Twig extensions refatoring to decouple definitions from implementations | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see twigphp/Twig#1913 | License | MIT | Doc PR | not yet This PR tries to use the new Twig runtime loader to the Twig bridge extensions. Commits ------- b515702 fixed circular reference in Twig Form integration 4b5e412 removed on indirection c07fc58 [TwigBridge] decoupled Form extension definitions from its runtime

… (chalasr) This PR was merged into the 1.x branch. Discussion ---------- Add a simple Twig_RuntimeLoaderInterface implementation Next to symfony/symfony#21023 This is related to the BC break reported in symfony/symfony#21008 which has been introduced in symfony/symfony#20093 when decoupling extensions from definitions. What I propose here is to ease the upgrade to symfony 3.2+ by adding a simple `Twig_RuntimeLoaderInterface` implementation here, useful only when using the twig-bridge outside of the symfony fullstack framework (with the Form component for instance). Upgrading would be as simple as: ```diff $twig = new Twig_Environment(...); $rendererEngine = new TwigRendererEngine(array('form_div_layout.html.twig'), $twig); - $twig->addExtension(new FormExtension(new TwigRenderer($rendererEngine, $csrfTokenManager))); + $twig->addExtension(new FormExtension()); + $twig->addRuntimeLoader(new Twig_RuntimeLoader(array(TwigRenderer::class => new TwigRenderer($rendererEngine, $csrfTokenManager))); ``` Instead of having to write this runtime loader yourself. Please see symfony/symfony#21008 for details and a concrete example of how this could help. Commits ------- 91c8d59 Add a Twig_FactoryRuntimeLoader

…tEnvironment() (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [TwigBridge] Late deprecation for TwigRendererEngine::setEnvironment() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20093 (comment) | License | MIT | Doc PR | n/a This method should have been deprecated in 3.2 since the twig environment should be injected through the constructor and replacing it later can break things (see #20093 (comment) for details). Maybe this could target 3.2 Commits ------- aabb73c Deprecate TwigRendererEngine::setEnvironment()

carsonbot added Status: Needs Review Feature labels Sep 29, 2016

fabpot force-pushed the form-extension-deprecations branch 3 times, most recently from 5253e33 to 27fd008 Compare September 29, 2016 19:46

linaori reviewed Sep 29, 2016

View reviewed changes

fabpot force-pushed the form-extension-deprecations branch from 27fd008 to 31fe9e2 Compare September 29, 2016 19:55

fabpot force-pushed the form-extension-deprecations branch 3 times, most recently from ea5a669 to 7973629 Compare September 29, 2016 21:13

fabpot force-pushed the form-extension-deprecations branch 2 times, most recently from 768427d to 0b76c8d Compare September 29, 2016 21:59

stof reviewed Sep 30, 2016

View reviewed changes

fabpot force-pushed the form-extension-deprecations branch 4 times, most recently from 182edfa to 22e45c0 Compare September 30, 2016 18:41

fabpot added 2 commits October 1, 2016 08:45

[TwigBridge] decoupled Form extension definitions from its runtime

c07fc58

removed on indirection

4b5e412

fabpot force-pushed the form-extension-deprecations branch from 22e45c0 to 2d01b80 Compare October 1, 2016 15:45

fixed circular reference in Twig Form integration

b515702

fabpot force-pushed the form-extension-deprecations branch from 2d01b80 to b515702 Compare October 1, 2016 16:08

fabpot merged commit b515702 into symfony:master Oct 1, 2016

fabpot deleted the form-extension-deprecations branch October 24, 2016 21:39

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

javiereguiluz mentioned this pull request Nov 25, 2016

Add docs for the new decoupled Twig extensions symfony/symfony-docs#7169

Closed

maidmaid mentioned this pull request Jul 6, 2017

[TwigBridge] Remove deprecated feature #23432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Twig extensions refatoring to decouple definitions from implementations #20093

Twig extensions refatoring to decouple definitions from implementations #20093

fabpot commented Sep 29, 2016

linaori Sep 29, 2016

fabpot Sep 29, 2016

linaori Sep 29, 2016

fabpot Sep 29, 2016

linaori Sep 29, 2016

fabpot Sep 29, 2016

linaori commented Sep 29, 2016

fabpot commented Sep 29, 2016

fabpot commented Sep 29, 2016

stof Sep 30, 2016

fabpot Sep 30, 2016

stof Sep 30, 2016

fabpot Sep 30, 2016

stof Sep 30, 2016

fabpot Sep 30, 2016

Twig extensions refatoring to decouple definitions from implementations #20093

Twig extensions refatoring to decouple definitions from implementations #20093

Conversation

fabpot commented Sep 29, 2016

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

linaori commented Sep 29, 2016

fabpot commented Sep 29, 2016

fabpot commented Sep 29, 2016

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