Problem/Motivation

Right now, Drupal only handles class::method notation to define controllers. As the controller is created by the controller resolver, a controller cannot its own constructor. This is by itself not a problem as you can get the container by implementing ContainerAwareInterface. But sometimes, you might want to control own your controllers are created, and using services for controllers allow you to do that.

Proposed resolution

This patch adds support for services as controllers with the same notation as Symfony itself. Use service_name:method instead of class::method.

Files: 
CommentFileSizeAuthor
#19 controller_resolver_19.patch2.83 KBfabpot
PASSED: [[SimpleTest]]: [MySQL] 50,877 pass(es).
[ View ]
#17 controller_resolver_17.patch2.74 KBfabpot
PASSED: [[SimpleTest]]: [MySQL] 49,735 pass(es).
[ View ]
#13 controller_resolver_13.patch2.75 KBfabpot
PASSED: [[SimpleTest]]: [MySQL] 50,613 pass(es).
[ View ]
#10 controller_resolver.patch2.76 KBfabpot
PASSED: [[SimpleTest]]: [MySQL] 50,065 pass(es).
[ View ]
#8 controller_resolver.patch2.65 KBfabpot
PASSED: [[SimpleTest]]: [MySQL] 50,028 pass(es).
[ View ]
#6 controller_resolver.patch2.65 KBfabpot
PASSED: [[SimpleTest]]: [MySQL] 49,805 pass(es).
[ View ]
#2 controller_resolver.patch2.65 KBfabpot
FAILED: [[SimpleTest]]: [MySQL] 49,574 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
controller_resolver.patch2.65 KBfabpot
FAILED: [[SimpleTest]]: [MySQL] 49,593 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, controller_resolver.patch, failed testing.

StatusFileSize
new2.65 KB
FAILED: [[SimpleTest]]: [MySQL] 49,574 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Resubmitting the patch

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, controller_resolver.patch, failed testing.

Status:Needs work» Needs review

Tests are failing randomly as the same patch does not end with the same failing tests.

StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 49,805 pass(es).
[ View ]

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -15,9 +15,15 @@
+ *    necessary, that allows a controller to vary the services it needs at runtime.
+ *
+ *  * By default, a controller name follows the class::method notation. This class
+ *    adds the possibility to use a service from the container as a controller by

Your comments exceed the 80 chars limit.

http://drupal.org/node/1354#general

Read code twice and I have no idea what it does :) So I'm leaving this for others.

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 50,028 pass(es).
[ View ]

Hopefully, the coding standards are fixed now.

Let me explain what this patch does with a simple example. Let say that you have a SomeModule module enabled.

In a routing configuration file, you associate a route pattern with a controller (the code that is executed when the route pattern matches). Here is a simple example:

hello:
  pattern: /hello/{name}
  defaults:
    _controller: Drupal\SomeModule\HelloController::helloAction

With the above configuration (stored in SomeModule.routing.yml), whenever you enter a URL that matches /hello/{name} (/hello/Bram for instance), the route matches and so the associated controller is executed. But the _controller value in the routing configuration file can only be a string, and the one used here is not a valid PHP callable.

So, the controller resolver is responsible for the creation of the PHP callable that should be executed (HttpKernel will then execute this PHP callable). Here, it will create an instance of Drupal\SomeModule\HelloController and return an array where the first element is the object and the second one the helloAction string.

Moreover, if the controller class implements ContainerAwareInterface, the container instance is injected by calling the setContainer() method (this is done by the controller resolver as well). This makes it possible for the controller to get access to any service from the container easily.

But sometimes, you want to control how the controller is created. In this case, instead of defining a controller like above (class::method), you can (thanks to this patch), define a method on a service (by using the service:method convention):

hello:
  pattern: /hello/{name}
  defaults:
    _controller: some_module.hello_controller::helloAction

Here, the controller resolver will get the controller from the container (the service name being some_module.hello_controller) instead of creating it directly. So, the controller will now be created by the container thanks to the definition you have defined in your SomeModuleBundle class.

Woow, this explanation helps a lot. You should create lots of issues ;).

1) I love the Exception handling.
2) In http://drupal.org/node/1354#classes there is a small example of exception documentation. Maybe you could adapt that standard in this patch.
3) Overall this makes sense.

+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -52,15 +60,28 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+    if (1 == $count = substr_count($controller, ':')) {

I'm not a fan of these string tricks. But I guess there isn't another way to check this. (I can't think of any)

StatusFileSize
new2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 50,065 pass(es).
[ View ]

I've added a phpdoc about the exception

Status:Needs review» Needs work

Firstly, I think this would be an awesome patch to get in. Having the ability to just swap out these in the container will be great!

A couple of things:

+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -15,9 +15,17 @@
+ *  * When creating a new object-based controller that implements

Nitpick alert: I think we use '-' instead of '*' characters here.

+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -50,17 +58,33 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+    if (false !== strpos($controller, '::')) {

I think usually we do these conditionals the other way around. I.e. (strpos(..) !== FALSE).

+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -50,17 +58,33 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+    if (1 == $count = substr_count($controller, ':')) {

Same as above and could we just lose the $count = part and just do substr_count($controller, ':') == 1 instead?

Title:Adds the possibility to use services as controllersAdd the possibility to use services as controllers

Status:Needs work» Needs review
StatusFileSize
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 50,613 pass(es).
[ View ]

I've fixed the CS.

Status:Needs review» Reviewed & tested by the community

Lukas would be proud of you. :-)

I'm going to RTBC this, but since it's a feature request it probably won't get committed for a little bit as we're over thresholds. I don't know if any existing work that would break it, though, so it's probably safe to sit for a bit.

Thanks, fabpot!

Agreed, this looks great now!

Category:feature» task
Status:Reviewed & tested by the community» Needs work

This looks great but I have minor nitpicks with the comments.
service:method here.

+ *    class adds the possibility to use a service from the container as a
+ *    controller by using a service:method notation (Symfony uses the same
+ *    convention).
  */

But service_name:method here.

+    // service_name:method

Should pick one :)

Also we usually shy away from one-word comments like this, but I think in this case it's explanatory enough.

Moving to a task as well.

Status:Needs work» Needs review
StatusFileSize
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 49,735 pass(es).
[ View ]

I've renamed everything to service:method

Should be ready now.

+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -50,17 +58,33 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+        throw new \InvalidArgumentException(sprintf('Class "%s" does not exist.', $class));

I missed this one. We should throw this to in the docs ;)

Shouldn't block a possible commit

StatusFileSize
new2.83 KB
PASSED: [[SimpleTest]]: [MySQL] 50,877 pass(es).
[ View ]

added a missing phpdoc

Status:Needs review» Reviewed & tested by the community

Nitpicks appear to have been addressed.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

I'm wondering whether there should be a changerecord for that issue?