Fix compatibility with Symfony 3.2 form renderer #4216

Koc · 2016-11-24T21:11:04Z

I am targetting this branch, because this is bugfix.

Changelog

### Added
- Added helper method `CRUDController::setFormTheme`.
### Fixed
- Fix compatibility with Symfony 3.2 form renderer.

Subject

Proposed sollution in #4154 breaks compatibility with older Symfony versions because twig.form.renderer service is private https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml#L156 . Also I've updated code in other places.

Koc · 2016-11-24T21:20:17Z

Also another approach is maybe calling

$this->get('twig')->getRuntime('Symfony\Bridge\Twig\Form\TwigRenderer')->setTheme(...)

@fabpot, @stof can you please suggest how to deal in this case?

greg0ire · 2016-11-24T21:28:18Z

Controller/CRUDController.php

@@ -105,7 +105,12 @@ public function listAction()
        $formView = $datagrid->getForm()->createView();

        // set the theme for the current Admin Form
-        $this->get('twig')->getExtension('Symfony\Bridge\Twig\Extension\FormExtension')->renderer->setTheme($formView, $this->admin->getFilterTheme());
+        if ($this->has('twig.form.renderer')) {


has does not seem to exist (see Travis)

hum, it actually does

greg0ire · 2016-11-24T21:28:29Z

Controller/CRUDController.php

+            $this->get('twig.form.renderer')->setTheme($formView, $this->admin->getFilterTheme());
+        } else {
+            // BC for Symfony < 3.2
+            $this->get('twig')->getExtension('Symfony\Bridge\Twig\Extension\FormExtension')->renderer->setTheme($formView, $this->admin->getFilterTheme());


Please linebreak this

greg0ire · 2016-11-24T21:33:55Z

Controller/HelperController.php

@@ -112,7 +112,13 @@ public function appendFormFieldElementAction(Request $request)

        $extension = $this->twig->getExtension('Symfony\Bridge\Twig\Extension\FormExtension');
        $extension->initRuntime($this->twig);
-        $extension->renderer->setTheme($view, $admin->getFormTheme());
+
+        if ($this->has('twig.form.renderer')) {


HelperController does not extend the standard sf controller, hence the error you get in Travis

You can get the container from the pool if you wish (it's a very ugly, but should do the trick).

The alternative would be to simply extend the standard controller.

No, I will inject twig.form.renderer service here inside constructor. Controller as a service helps a lot here.

~~Ok, but you will have do inject it conditionally, since it does not necessarily exist, right?~~ my bad there is always a renderer

It exists since 2.3 (or even earlier but we support >= 2.3) https://github.com/symfony/symfony/blob/2.3/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml#L102 but it was private service before 3.2. Private services cann't be retrieved at runtime but they can properly injected into another services.

greg0ire · 2016-11-24T22:27:22Z

Tests/Controller/HelperControllerTest.php

@@ -717,4 +717,14 @@ public function getValidatorInterfaces()

        return $data;
    }
+
+    protected function getTwigRenderer()


core23 · 2016-11-25T06:03:02Z

Controller/CRUDController.php

@@ -1364,4 +1363,18 @@ protected function preList(Request $request)

        return $this->get('translator')->trans($id, $parameters, $domain, $locale);
    }
+
+    protected function setFormTheme(FormView $formView)


PHPdoc missing

Would prefer a private method, because we will move the twig stuff to the template completly.

core23 · 2016-11-25T06:03:38Z

Controller/HelperController.php

     */
-    public function __construct(\Twig_Environment $twig, Pool $pool, AdminHelper $helper, $validator)
+    public function __construct(\Twig_Environment $twig, Pool $pool, AdminHelper $helper, $validator, TwigRenderer $twigRenderer)


This is a BC break. Please make the last parameter optional

And use TwigRendererInterface as type hinting, it will be easier to make the build pass

@greg0ire it is deprecated since 3.2 https://github.com/symfony/twig-bridge/blob/master/Form/TwigRendererInterface.php

Oh… too bad.

OskarStark

We need to stay BC here, this is a BC break!

core23 · 2016-11-30T19:19:42Z

Controller/CRUDController.php

@@ -1348,6 +1347,23 @@ protected function preList(Request $request)
    {
    }

+    protected function setFormTheme(FormView $formView)


private please

add phpdoc

core23 · 2016-11-30T19:19:51Z

Controller/HelperController.php

@@ -497,4 +493,18 @@ private function retrieveFilterFieldDescription(AdminInterface $admin, $field)

        return $fieldDescription;
    }
+
+    protected function setFormTheme(FormView $formView, $theme)


private please

add phpdoc

fbourigault · 2016-12-01T09:12:50Z

This is a Symfony BC break. I opened a pull request: symfony/symfony#20710.

cedricziel · 2016-12-01T14:49:48Z

Just saw this PR. FYI: I have a similar approach, but use the container to retrieve the renderer, which should save some hassle. cedricziel@23707f9

Koc · 2016-12-01T15:18:13Z

@cedricziel I've tryed this before. The problem that we need pass another dependency to HelperController. In my updated sollution we using only Twig.

cedricziel · 2016-12-01T15:20:00Z

@Koc the HelperController in my commit uses twig itself or falls back on the old way. cedricziel@23707f9#diff-bca3ecd7f88753148a6fec5ea1b4623aR446

greg0ire · 2016-12-02T09:06:44Z

@Koc are you stuck? If yes how can we help? Should we ask @cedricziel to make a PR?

Koc · 2016-12-02T09:30:47Z

@greg0ire just need more time to fix tests

…ct#4152, sonata-project#4154).

core23 · 2016-12-02T10:15:11Z

Controller/CRUDController.php

+     *
+     * @param FormView $formView
+     */
+    protected function setFormTheme(FormView $formView)


We still need a private method

This method can be useful for custom actions that extends from crud controller and used forms. We have a lot setFormTheme calls in our actions and for now they also not works anymore.

That's true, but we will remove the feature to set the form rendering in the controller. This will be done in the template files with the next major.

core23 · 2016-12-02T10:16:08Z

Tests/Controller/HelperControllerTest.php

@@ -328,6 +329,20 @@ public function testappendFormFieldElementAction($validatorInterface)

        $twig = new \Twig_Environment($this->getMock('\Twig_LoaderInterface'));
        $twig->addExtension(new FormExtension($mockRenderer));
+
+        if (Kernel::MAJOR_VERSION >= 3 && Kernel::MINOR_VERSION >= 2) {


Not sure, if we could rely on the kernel version.

cc @greg0ire

We shouldn't. I'm currently waiting for the tests to pass, and then I think we should do a complete review, but yeah this is not the way to go. Detecting the presence of the public property might be a solution, or anything else that changed since 3.2 if it is too hard (the property does not exist at all times).

Question: Why do you think it might be a bad idea to rely on Kernel to bring the version information? Maybe I'm mistaken, but those fields provide exactly the info we need. I'd rather switch to Kernel::VERSION_ID >= 30200 though.

Here is your answer right from the sf docs : https://symfony.com/doc/current/setup/bundles.html#updating-your-code-to-support-symfony-2-x-and-3-x-at-the-same-time

Interesting-thank you for the link.

You're very welcome :)

@cedricziel the bundle depends on individual packages, not on the fullstack repo (to give flexibility to users), but this allows to have different versions of each package. Kernel constants give you the version of HttpKernel, not of the Twig bridge

Based on this changelog, here is the detection trick I'd use

wouterj · 2016-12-04T00:28:19Z

Why not simply doing $this->get('twig.form.renderer')->setTheme(...) ? Anywhy, please fix Symfony 3.2 support soon :)

nicolas-grekas · 2016-12-05T17:57:39Z

Please test symfony/symfony#20769

greg0ire · 2016-12-09T13:18:17Z

Triggering a new build. Let's pray ⛪

greg0ire · 2016-12-09T13:32:01Z

Let's not celebrate prematurely, but it looks good :)

greg0ire · 2016-12-09T13:41:18Z

@Koc please fix the StyleCI build

greg0ire · 2016-12-09T13:46:28Z

Aaaand it's green! Congrats!

Koc · 2016-12-09T13:48:03Z

@greg0ire done

Koc · 2016-12-09T13:48:50Z

Have someone tested it in real life application (not only unit tests)? does it really works?

greg0ire · 2016-12-09T13:49:50Z

@wouterj maybe you can? I don't have such an app lying around.

greg0ire · 2016-12-09T13:57:00Z

Tests/Controller/CRUDControllerTest.php

+            ->method('getRuntime')
+            ->will($this->returnCallback(function ($name) use ($twigRenderer) {
+                switch ($name) {
+                    case 'Symfony\Bridge\Twig\Form\TwigRenderer':


A switch with one case… 😕

cedricziel · 2016-12-09T14:00:55Z

Just test-drove it in a (rather minimal) sf 3.2 app. All nice and dandy so far.

requested changes were addressed

greg0ire · 2016-12-09T14:24:12Z

@core23 @OskarStark please give this a final review and merge

wouterj

Tested this PR in the CMF test jobs and previously failing builds are now fixed. So I can confirm this fixes the bug.

greg0ire · 2016-12-09T16:16:31Z

@core23 @OskarStark this needs to be squash-merged (or at least, some of the commits should be squashed together)

greg0ire · 2016-12-12T10:08:11Z

@sonata-project/contributors : this is kind of a blocker. I'll merge this afternoon if you don't have anything to say.

greg0ire · 2016-12-12T13:14:18Z

Squashed with a new commit message

greg0ire · 2016-12-12T13:20:42Z

Thanks @Koc ! And thanks everyone for testing / reviews !

greg0ire · 2016-12-13T14:00:12Z

3.10.1 has been released with this fix in it!

axzx · 2016-12-14T10:10:10Z

Do you know, that this code

// set the theme for the current Admin Form
$this->get('twig')->getExtension('Symfony\Bridge\Twig\Extension\FormExtension')
          ->renderer->setTheme($view, $this->admin->getFormTheme());

works fine in Symfony 3.2.1?

stof · 2016-12-14T10:42:55Z

@axzx but it triggers deprecation warnings

…4216) Our code was relying on an undeclared private property of the form Twig extension to get a renderer object, in order to set the form theme. On recent versions of Symfony, the renderer is now obtained from the Twig environment.

SonataCI added the review required label Nov 24, 2016

greg0ire requested changes Nov 24, 2016

View reviewed changes

greg0ire mentioned this pull request Nov 24, 2016

#4152 fixed trying to access private property from CRUD Controller #4154

Closed

3 tasks

greg0ire reviewed Nov 24, 2016

View reviewed changes

Koc changed the title ~~Fix compatibility with Symfony 3.2 form renderer~~ [WIP] Fix compatibility with Symfony 3.2 form renderer Nov 24, 2016

greg0ire reviewed Nov 24, 2016

View reviewed changes

Koc force-pushed the fix-symfony-compatibility branch from 1fc343c to f5a7885 Compare November 24, 2016 22:33

core23 requested changes Nov 25, 2016

View reviewed changes

OskarStark previously requested changes Nov 25, 2016

View reviewed changes

OskarStark added the patch label Nov 25, 2016

Koc force-pushed the fix-symfony-compatibility branch from d1a37d8 to 512b4d5 Compare November 30, 2016 17:21

core23 requested changes Nov 30, 2016

View reviewed changes

fbourigault mentioned this pull request Dec 2, 2016

[TwigBridge] fix FormExtension::$renderer bc break symfony/symfony#20710

Closed

Koc force-pushed the fix-symfony-compatibility branch from 512b4d5 to 6ee4761 Compare December 2, 2016 09:25

Fix compatibility with Symfony 3.2 form renderer (closes sonata-proje…

513116b

…ct#4152, sonata-project#4154).

Koc force-pushed the fix-symfony-compatibility branch from 6ee4761 to 513116b Compare December 2, 2016 09:37

core23 previously requested changes Dec 2, 2016

View reviewed changes

wouterj mentioned this pull request Dec 4, 2016

Make Build Green symfony-cmf/sonata-phpcr-admin-integration-bundle#11

Closed

greg0ire mentioned this pull request Dec 5, 2016

CRUD Controller tries to access private property Symfony\Bridge\Twig\Extension\FormExtension::$renderer #4152

Closed

nicolas-grekas mentioned this pull request Dec 5, 2016

[Bridge\Twig] Trigger deprecation when using FormExtension::$renderer symfony/symfony#20769

Merged

greg0ire mentioned this pull request Dec 5, 2016

Adjust role permission #4221

Merged

greg0ire added the critical label Dec 7, 2016

greg0ire mentioned this pull request Dec 8, 2016

Dropdown arrows position corrected #4219

Merged

greg0ire closed this Dec 9, 2016

greg0ire reopened this Dec 9, 2016

Fix styleci build

fdd1b24

greg0ire reviewed Dec 9, 2016

View reviewed changes

greg0ire approved these changes Dec 9, 2016

View reviewed changes

Koc changed the title ~~[WIP] Fix compatibility with Symfony 3.2 form renderer~~ Fix compatibility with Symfony 3.2 form renderer Dec 9, 2016

wouterj approved these changes Dec 9, 2016

View reviewed changes

greg0ire merged commit d8fc2ed into sonata-project:3.x Dec 12, 2016

OskarStark removed the review required label Dec 13, 2016

Fix compatibility with Symfony 3.2 form renderer #4216

Fix compatibility with Symfony 3.2 form renderer #4216

Conversation

Koc commented Nov 24, 2016 • edited

Changelog

Subject

Koc commented Nov 24, 2016 • edited by OskarStark

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

Choose a reason for hiding this comment

greg0ire Nov 24, 2016 • edited

Choose a reason for hiding this comment

Koc Nov 24, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault commented Dec 1, 2016

cedricziel commented Dec 1, 2016

Koc commented Dec 1, 2016

cedricziel commented Dec 1, 2016

greg0ire commented Dec 2, 2016

Koc commented Dec 2, 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

greg0ire Dec 2, 2016 • edited

Choose a reason for hiding this comment

cedricziel Dec 2, 2016 • edited

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

wouterj commented Dec 4, 2016

nicolas-grekas commented Dec 5, 2016

greg0ire commented Dec 9, 2016

greg0ire commented Dec 9, 2016

greg0ire commented Dec 9, 2016

greg0ire commented Dec 9, 2016

Koc commented Dec 9, 2016

Koc commented Dec 9, 2016 • edited

greg0ire commented Dec 9, 2016

Choose a reason for hiding this comment

cedricziel commented Dec 9, 2016

greg0ire commented Dec 9, 2016

wouterj left a comment

Choose a reason for hiding this comment

greg0ire commented Dec 9, 2016

greg0ire commented Dec 12, 2016

greg0ire commented Dec 12, 2016

greg0ire commented Dec 12, 2016

greg0ire commented Dec 13, 2016 • edited

axzx commented Dec 14, 2016

stof commented Dec 14, 2016

Koc commented Nov 24, 2016 •

edited

Koc commented Nov 24, 2016 •

edited by OskarStark

greg0ire Nov 24, 2016 •

edited

Koc Nov 24, 2016 •

edited

greg0ire Dec 2, 2016 •

edited

cedricziel Dec 2, 2016 •

edited

Koc commented Dec 9, 2016 •

edited

greg0ire commented Dec 13, 2016 •

edited