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.

CommentFileSizeAuthor
#19 controller_resolver_19.patch2.83 KBfabpot
#17 controller_resolver_17.patch2.74 KBfabpot
#13 controller_resolver_13.patch2.75 KBfabpot
#10 controller_resolver.patch2.76 KBfabpot
#8 controller_resolver.patch2.65 KBfabpot
#6 controller_resolver.patch2.65 KBfabpot
#2 controller_resolver.patch2.65 KBfabpot
controller_resolver.patch2.65 KBfabpot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

fabpot’s picture

fabpot CreditAttribution: fabpot commented
FileSize
controller_resolver.patch2.65 KB

Resubmitting the patch

fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review

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

fabpot’s picture

fabpot CreditAttribution: fabpot commented
FileSize
controller_resolver.patch2.65 KB
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
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.

fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review
FileSize
controller_resolver.patch2.65 KB

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.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

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)

fabpot’s picture

fabpot CreditAttribution: fabpot commented
FileSize
controller_resolver.patch2.76 KB

I've added a phpdoc about the exception

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
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?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Title: Adds the possibility to use services as controllers » Add the possibility to use services as controllers
fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review
FileSize
controller_resolver_13.patch2.75 KB

I've fixed the CS.

Crell’s picture

Crell CreditAttribution: Crell commented
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!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Agreed, this looks great now!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
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.

fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review
FileSize
controller_resolver_17.patch2.74 KB

I've renamed everything to service:method

Should be ready now.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented 
+++ 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

fabpot’s picture

fabpot CreditAttribution: fabpot commented
FileSize
controller_resolver_19.patch2.83 KB

added a missing phpdoc

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Nitpicks appear to have been addressed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

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