[Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait #24409

nicolas-grekas · 2017-10-03T13:24:00Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

With this PR, the last two remaining uses of ContainerAwareTrait will be Symfony\Component\HttpKernel\Bundle\Bundle and Symfony\Bundle\FrameworkBundle\Controller\Controller.
For Bundle, I think it's legitimate, for Controller, I think it's not, but that we should wait for 4.1 before considering its deprecation, alongside with ContainerAwareCommand (maybe).

fabpot · 2017-10-03T13:50:46Z

src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php

+    private $httpPort;
+    private $httpsPort;
+
+    public function __construct(ContainerInterface $container = null, $httpPort = null, $httpsPort = null)


What about passing a UrlGenerator here instead of a container?

fabpot · 2017-10-03T13:51:32Z

src/Symfony/Bundle/FrameworkBundle/Controller/TemplateController.php

-    use ContainerAwareTrait;
+    protected $container;
+
+    public function __construct(ContainerInterface $container = null)


Same as for the redirect one. What about passing a Twig and an Engine here instead. By the way, we should probably also deprecate the templating path.

nicolas-grekas · 2017-10-03T14:48:36Z

Comments addressed.

Status: needs review

fabpot · 2017-10-03T14:53:24Z

src/Symfony/Bundle/FrameworkBundle/Controller/TemplateController.php

+    private $templating;
+    private $twig;
+
+    public function __construct(EngineInterface $templating = null, Environment $twig = null)


I would put Twig first as the other one will be deprecated.

Tobion · 2017-10-03T15:23:51Z

src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php

+     */
+    public function setContainer(ContainerInterface $container = null)
+    {
+        @trigger_error(sprintf('The "%s()" method is deprecated since version 3.4 and will be removed in 4.0. Inject a PSR-11 container using the constructor instead.', __METHOD__), E_USER_DEPRECATED);


huh? the constructor does not accept a container.

Tobion · 2017-10-03T15:30:42Z

src/Symfony/Bridge/Doctrine/ManagerRegistry.php

+     */
+    public function setContainer(SymfonyContainerInterface $container = null)
+    {
+        @trigger_error(sprintf('The "%s()" method is deprecated since version 3.4 and will be removed in 4.0. Inject a PSR-11 container using the constructor instead.', __METHOD__), E_USER_DEPRECATED);


abstract ManagerRegistry does not accept a container in its argument. only \Doctrine\Bundle\DoctrineBundle\Registry does. also https://github.com/doctrine/DoctrineBundle/blob/master/Registry.php#L41 still calls the deprecated method

Yes, I'll do the PR next on DoctrineBundle.

Let's wait for the PR on DoctrineBundle before merging this one.

Tobion · 2017-10-03T17:53:32Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml

+            <argument>%request_listener.https_port%</argument>
+        </service>
+
+        <service id="Symfony\Bundle\FrameworkBundle\Controller\TemplateController" public="true">


I don't think this service belongs in routing.xml. It has nothing to do with routing?!

These services are useful only when a route references them, that's why I made it this way.
Did I miss something else?

Can't one use them without routing using fragments?

nicolas-grekas · 2017-10-03T19:30:34Z

Here is the doctrine-bundle PR:
doctrine/DoctrineBundle#710

stof · 2017-10-05T08:39:58Z

src/Symfony/Bundle/FrameworkBundle/Controller/TemplateController.php

-        } elseif ($this->container->has('twig')) {
-            $response = new Response($this->container->get('twig')->render($template));
+        if ($this->templating) {
+            $response = $this->templating->renderResponse($template);


the interface of the component, that you used in the typehint does not have the renderResponse method

good catch, fixed, thanks

I would have done the opposite. Not relying on the interface from frameworkbundle and create the response by hand as done for Twig.

chalasr · 2017-10-05T08:46:38Z

src/Symfony/Bridge/Doctrine/ManagerRegistry.php

+
+    /**
+     * @deprecated since version 3.4, to be removed in 4.0 alongside with the ContainerAwareInterface type.
+     * @final since version 3.4


can't it just be deprecated?

I think so: @deprecated doesn't trigger a deprecation when a method is overridden but the parent is not called. @final does.

chalasr · 2017-10-05T08:48:21Z

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/TemplateControllerTest.php

+
+    public function testTemplating()
+    {
+        $templating = $this->getMockBuilder('Symfony\Bundle\FrameworkBundle\Templating\EngineInterface')->getMock();


could use ::class

fabpot · 2017-10-05T12:13:58Z

src/Symfony/Bundle/FrameworkBundle/Controller/TemplateController.php

-        } elseif ($this->container->has('twig')) {
-            $response = new Response($this->container->get('twig')->render($template));
+        if ($this->templating) {
+            $response = $this->templating->renderResponse($template);


I would have done the opposite. Not relying on the interface from frameworkbundle and create the response by hand as done for Twig.

fabpot · 2017-10-05T12:14:17Z

src/Symfony/Bundle/FrameworkBundle/Controller/TemplateController.php

-            $response = new Response($this->container->get('twig')->render($template));
+        if ($this->templating) {
+            $response = $this->templating->renderResponse($template);
+        } elseif ($this->twig) {


Let's check Twig first :)

That'd be bc breaking changes...

I agree with this one, but not the other one.

nicolas-grekas · 2017-10-05T13:31:16Z

@fabpot comments addressed

Status: needs review

…ontainerAwareTrait

fabpot · 2017-10-05T14:35:39Z

Thank you @nicolas-grekas.

…ining uses of ContainerAwareTrait (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - With this PR, the last two remaining uses of ContainerAwareTrait will be `Symfony\Component\HttpKernel\Bundle\Bundle` and `Symfony\Bundle\FrameworkBundle\Controller\Controller`. For Bundle, I think it's legitimate, for Controller, I think it's not, but that we should wait for 4.1 before considering its deprecation, alongside with `ContainerAwareCommand` (maybe). Commits ------- df9c874 [Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait

…of ContainerAwareInterface (nicolas-grekas) This PR was merged into the 4.0-dev branch. Discussion ---------- [Bridge\Doctrine][FrameworkBundle] Remove legacy uses of ContainerAwareInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Related to #24409 Commits ------- 3b1b8cf [Bridge\Doctrine][FrameworkBundle] Remove legacy uses of ContainerAwareInterface

nicolas-grekas added this to the 3.4 milestone Oct 3, 2017

carsonbot added Status: Needs Review FrameworkBundle Deprecation labels Oct 3, 2017

nicolas-grekas force-pushed the psr11 branch from 102f3c7 to 6bf122c Compare October 3, 2017 13:27

fabpot requested changes Oct 3, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 3, 2017

nicolas-grekas force-pushed the psr11 branch from 6bf122c to 77ffe6d Compare October 3, 2017 14:46

carsonbot added Status: Needs Review and removed Status: Needs Work Status: Needs Review labels Oct 3, 2017

fabpot requested changes Oct 3, 2017

View reviewed changes

carsonbot added the Status: Needs Work label Oct 3, 2017

nicolas-grekas force-pushed the psr11 branch from 77ffe6d to 4693423 Compare October 3, 2017 14:55

Simperfit approved these changes Oct 3, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 3, 2017

Tobion reviewed Oct 3, 2017

View reviewed changes

nicolas-grekas force-pushed the psr11 branch from 4693423 to 10fbaca Compare October 3, 2017 15:32

Tobion reviewed Oct 3, 2017

View reviewed changes

nicolas-grekas force-pushed the psr11 branch from 10fbaca to 4e5282a Compare October 3, 2017 18:33

nicolas-grekas mentioned this pull request Oct 3, 2017

Fix deprecation with Symfony 3.4 doctrine/DoctrineBundle#710

Merged

stof requested changes Oct 5, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 5, 2017

nicolas-grekas force-pushed the psr11 branch from 4e5282a to 39b1f2d Compare October 5, 2017 08:43

chalasr reviewed Oct 5, 2017

View reviewed changes

nicolas-grekas force-pushed the psr11 branch from 39b1f2d to d1e424c Compare October 5, 2017 09:00

chalasr added the DoctrineBridge label Oct 5, 2017

fabpot requested changes Oct 5, 2017

View reviewed changes

nicolas-grekas force-pushed the psr11 branch from d1e424c to a8e7382 Compare October 5, 2017 13:30

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 5, 2017

nicolas-grekas force-pushed the psr11 branch 2 times, most recently from 6666c84 to d0407ec Compare October 5, 2017 13:59

[Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of C…

df9c874

…ontainerAwareTrait

nicolas-grekas force-pushed the psr11 branch from d0407ec to df9c874 Compare October 5, 2017 14:01

fabpot merged commit df9c874 into symfony:3.4 Oct 5, 2017

nicolas-grekas deleted the psr11 branch October 5, 2017 14:36

This was referenced Oct 5, 2017

[Travis] Do not use legacy mongo ext #24440

Closed

[Bridge\Doctrine][FrameworkBundle] Remove legacy uses of ContainerAwareInterface #24441

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Tobion mentioned this pull request Jan 23, 2018

[RFC] deprecate bundle:controller:action notation #25910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait #24409

[Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait #24409

nicolas-grekas commented Oct 3, 2017

fabpot Oct 3, 2017

fabpot Oct 3, 2017

nicolas-grekas commented Oct 3, 2017

fabpot Oct 3, 2017

nicolas-grekas Oct 3, 2017

Tobion Oct 3, 2017

nicolas-grekas Oct 3, 2017

Tobion Oct 3, 2017

nicolas-grekas Oct 3, 2017

fabpot Oct 3, 2017

Tobion Oct 3, 2017

nicolas-grekas Oct 3, 2017

Tobion Oct 5, 2017

nicolas-grekas commented Oct 3, 2017

stof Oct 5, 2017

nicolas-grekas Oct 5, 2017

fabpot Oct 5, 2017

chalasr Oct 5, 2017

nicolas-grekas Oct 5, 2017

chalasr Oct 5, 2017

nicolas-grekas Oct 5, 2017

fabpot Oct 5, 2017

fabpot Oct 5, 2017

nicolas-grekas Oct 5, 2017

fabpot Oct 5, 2017

nicolas-grekas commented Oct 5, 2017

fabpot commented Oct 5, 2017

[Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait #24409

[Bridge\Doctrine][FrameworkBundle] Deprecate some remaining uses of ContainerAwareTrait #24409

Conversation

nicolas-grekas commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 5, 2017

fabpot commented Oct 5, 2017