FOSRestController is now a trait and not longer a class extending from the core Controller class #1207

lsmith77 · 2015-11-13T14:07:45Z

No description provided.

GromNaN · 2015-11-13T14:15:25Z

👍 As PHP 5.4 is required.

GuilhemN · 2015-11-13T14:17:44Z

I'm wondering if it wouldn't be better to suffix its name with Trait and to remove FOS: RestControllerTrait.

lsmith77 · 2015-11-13T14:18:30Z

yeah .. was pondering this as well

GuilhemN · 2015-11-13T14:20:48Z

And IMO we should create a last release for the 1.* branch with bc layers to help to migrate to FOSRestBundle 2.0.

lsmith77 · 2015-11-13T14:22:12Z

not sure if its worth it .. I mean we have tons of open bugs/enhancement tickets .. at this point moving to 2.0 should not take all too long .. what does make sense is to maybe invest some more time into a migration guide

GuilhemN · 2015-11-13T15:27:54Z

UPGRADING.md

@@ -16,6 +16,7 @@ This document will be updated to list important BC breaks and behavioral changes
 * Removed ``callback_filter`` configuration option for the jsonp_handler
 * ``exception_wrapper_handler`` is now the name of a service and not the name of a class
 * removed all ``.class`` parameters, instead overwriting services via explicit Bundle configuration is preferred
+ * ControllerTrait is now a trait and not longer a class extending from the core Controller class


This is not clear, imo include the new and the old name would be clearer.

GuilhemN · 2015-11-13T15:31:07Z

Controller/ControllerTrait.php

 */
-abstract class FOSRestController extends Controller
+trait ControllerTrait


I think that this trait must extend the ContainerAwareTrait as it can't work without it.

Another solution would be to add an abstract method get.

well the trait falls back to using the magic get() method .. but it can work without it ..
I am not quite sure how to best approach this .. maybe provide two traits .. ?

Oops look too fast...
If there are two traits, the problem will be the same (moreover get belongs to Controller) ...
You can maybe check if the get method exists on $this or if $this is an instance of Controller.

i fixed it by using $this->container->get()

Finally, I'm wondering if two traits it's not too complicated to use...
Can't we just check if $this is an instance of ContainerAwareInterface or if $this->viewHandler is defined and otherwise throw an exception?

lsmith77 · 2015-11-14T11:36:09Z

Controller/ControllerTrait.php

+    /**
+     * Get the ViewHandler.
+     *
+     * @@param View $view


there is a typo here that I need to fix

lsmith77 · 2015-11-14T13:52:10Z

To fix the tests one needs to do the following, which sucks big time as it basically means the trait isn't a logical unit anymore but requires other things that I cannot express with code :-/

diff --git a/Controller/ContainerAwareControllerTrait.php b/Controller/ContainerAwareControllerTrait.php
index b03ecd2..d677c1d 100644
--- a/Controller/ContainerAwareControllerTrait.php
+++ b/Controller/ContainerAwareControllerTrait.php
@@ -12,7 +12,6 @@
 namespace FOS\RestBundle\Controller;

 use FOS\RestBundle\View\ViewHandlerInterface;
-use Symfony\Component\DependencyInjection\ContainerAwareTrait;

 /**
  * Trait for Controllers using the View functionality of FOSRestBundle.
@@ -22,7 +21,6 @@ use Symfony\Component\DependencyInjection\ContainerAwareTrait;
 trait ContainerAwareControllerTrait
 {
     use ControllerTrait;
-    use ContainerAwareTrait;

     /**
      * Get the ViewHandler.

lsmith77 · 2015-11-14T14:01:34Z

Note even if we would change the core ContainerAware class to use the ContainerAwareTrait we would still run into the same problem as can be seen here https://3v4l.org/l1qkE

lsmith77 · 2015-11-14T14:16:56Z

ok changing to use getContainer() and defining such an abstract method within ContrainerAwareControllerTrait along with the following change fixes the issue:

diff --git a/ContainerAware.php b/ContainerAware.php
index af977fe..080f8cb 100644
--- a/ContainerAware.php
+++ b/ContainerAware.php
@@ -18,18 +18,5 @@ namespace Symfony\Component\DependencyInjection;
  */
 abstract class ContainerAware implements ContainerAwareInterface
 {
-    /**
-     * @var ContainerInterface
-     */
-    protected $container;
-
-    /**
-     * Sets the Container associated with this Controller.
-     *
-     * @param ContainerInterface $container A ContainerInterface instance
-     */
-    public function setContainer(ContainerInterface $container = null)
-    {
-        $this->container = $container;
-    }
+    use ContainerAwareTrait;
 }
diff --git a/ContainerAwareTrait.php b/ContainerAwareTrait.php
index 57280aa..caf222a 100644
--- a/ContainerAwareTrait.php
+++ b/ContainerAwareTrait.php
@@ -32,4 +32,14 @@ trait ContainerAwareTrait
     {
         $this->container = $container;
     }
+
+    /**
+     * Gets the Container associated with this Controller.
+     *
+     * @return ContainerInterface
+     */
+    public function getContainer()
+    {
+        return $this->container;
+    }
 }

Note technically it is sufficient to add the getContainer() method to ContainerAware but while we are at it, we should also add such a method to ContainerAwareTrait. And we can of course choose to not use the ContrainerAwareTrait inside the ContainerAware class in all versions of Symfony that are compatible with PHP 5.3.

tl;dr: trait properties should always come with a getter method

GuilhemN · 2015-11-14T15:39:57Z

What about doing something like https://github.com/FriendsOfSymfony/FOSRestBundle/compare/master...Ener-Getick:CONTROLLER?expand=1 ?

lsmith77 · 2015-11-14T19:49:49Z

@Ener-Getick at the end of the day it still relies on a property that is no where defined in the trait or the contract defined by the trait. will see what happens with the ticket I opened on symfony core.

lsmith77 · 2015-12-04T18:17:56Z

Controller/ContainerAwareControllerTrait.php

+ *
+ * @author Lukas Kahwe Smith <smith@pooteeweet.org>
+ */
+trait ContainerAwareControllerTrait


refactor this to be a base class, ie. keep the existing name FOSRestController

lsmith77 · 2015-12-05T12:16:10Z

ok .. keeping FOSRestController and just introducing ControllerTrait .. ready for final review.

GuilhemN · 2015-12-05T12:20:31Z

Controller/ControllerTrait.php

+     *
+     * @param ViewHandlerInterface $viewhandler
+     */
+    protected function setViewHandler(ViewHandlerInterface $viewhandler)


IMO this should be public to allow setter injection

GuilhemN · 2015-12-07T20:30:31Z

Except my last comment, sounds good to me 👍.

lsmith77 · 2015-12-08T07:16:54Z

@Ener-Getick fixed

…r controllers instead of extending ``FOSRestController``

FOSRestController is now a trait and not longer a class extending from the core Controller class

lsmith77 added the wip/poc label Nov 13, 2015

lsmith77 force-pushed the make_controller_a_trait branch from 41e0181 to 4aafa19 Compare November 13, 2015 14:08

lsmith77 added in review and removed wip/poc labels Nov 13, 2015

lsmith77 added this to the 2.0 milestone Nov 13, 2015

lsmith77 force-pushed the make_controller_a_trait branch 2 times, most recently from c9a3762 to 7825c35 Compare November 13, 2015 15:10

GuilhemN reviewed Nov 13, 2015
View reviewed changes

lsmith77 force-pushed the make_controller_a_trait branch from 7825c35 to cd331d8 Compare November 13, 2015 15:30

GuilhemN reviewed Nov 13, 2015
View reviewed changes

lsmith77 force-pushed the make_controller_a_trait branch 3 times, most recently from 675cdb6 to f27465f Compare November 13, 2015 17:41

lsmith77 reviewed Nov 14, 2015
View reviewed changes

Controller/ControllerTrait.php

/**

* Get the ViewHandler.

*

* @@param View $view

Copy link

Member Author

lsmith77 Nov 14, 2015

Choose a reason for hiding this comment

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

there is a typo here that I need to fix

lsmith77 force-pushed the make_controller_a_trait branch from f27465f to 2be0336 Compare November 14, 2015 11:54

lsmith77 mentioned this pull request Nov 14, 2015

make sure that all traits with properties have accessors symfony/symfony#16548

Closed

lsmith77 reviewed Dec 4, 2015
View reviewed changes

lsmith77 force-pushed the make_controller_a_trait branch from 76a649b to f5bbe57 Compare December 5, 2015 12:12

lsmith77 force-pushed the make_controller_a_trait branch 3 times, most recently from aee479f to 2393d31 Compare December 5, 2015 12:15

GuilhemN reviewed Dec 5, 2015
View reviewed changes

lsmith77 force-pushed the make_controller_a_trait branch 2 times, most recently from 8ee4666 to 26c5bc2 Compare December 8, 2015 07:16

added ControllerTrait for developers that prefer to use DI for thei…

7970b0c

…r controllers instead of extending ``FOSRestController``

lsmith77 force-pushed the make_controller_a_trait branch from 26c5bc2 to 7970b0c Compare December 8, 2015 08:02

lsmith77 added a commit that referenced this pull request Dec 8, 2015

Merge pull request #1207 from FriendsOfSymfony/make_controller_a_trait

78698c1

FOSRestController is now a trait and not longer a class extending from the core Controller class

lsmith77 merged commit 78698c1 into master Dec 8, 2015

lsmith77 removed the in review label Dec 8, 2015

lsmith77 deleted the make_controller_a_trait branch December 8, 2015 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

FOSRestController is now a trait and not longer a class extending from the core Controller class #1207

FOSRestController is now a trait and not longer a class extending from the core Controller class #1207

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FOSRestController is now a trait and not longer a class extending from the core Controller class #1207

FOSRestController is now a trait and not longer a class extending from the core Controller class #1207

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!