Spin off from #1801356: Entity reference autocomplete using routes. That issue surfaced a debate on whether route controllers (the _controller and _content values of routes) should reference explicit classes, for example, this snippet from edit.module's routing file:

edit_field_form:
  pattern: '/edit/form/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode}'
  defaults:
    _controller: '\Drupal\edit\EditController::fieldForm'

Or, whether they should reference services, as in this snippet from views_ui.module's routing file:

views_ui.list:
  pattern: '/admin/structure/views'
  defaults:
    _controller: 'views_ui.controller:listing'

Where views_ui.controller is a service registered in ViewsUiBundle::build().

In #1801356-62: Entity reference autocomplete using routes, Dries requested that in core, we pick one approach, instead of mixing them. Since both are supported by the Symfony routing system we're using, contrib can choose to use either, but this is about improving DX in core by standardizing.

The main benefit of registering controllers as services is that dependencies can be made explicit. For example, the ViewsUIController above has a constructor that receives an EntityManager, a ViewsDataCache, and a TempStoreFactory. Anyone reading that code gets immediate knowledge of that, and anyone writing code to use ViewsUIController (for example, a unit test) can quickly figure out what that class requires to be injected into it. Whereas, the EditController that isn't a service has no such constructor, so the only way to know its dependencies is to open up the code and scan for wherever $this->container->get() is called.

However, this benefit is of less value than with our other services. Because the entire point of controllers is to *only* be the glue code between a route and a response. Any reusable functionality needed in that process should be implemented by a different class, and one that is a service. So, there's little value in catering to someone who wants to work with a controller outside of its routing context. I would even suggest that controller unit tests are of minimal value and we shouldn't have any. We need to unit test the underlying services used by controllers, and we need to functional test routes. If we do both of those, unit testing controllers is pretty pointless.

The main concern of registering controllers as services is adding weight to the DIC. The DIC needs to be loaded on every page request, and Drupal sites might commonly have hundreds/thousands of routes/controllers. However, we don't yet have enough of Drupal converted to the new routing system to evaluate this empirically.

Discuss.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

The code in views_ui.controller is a temporary measure, until #1913856: Convert Views UI forms to use FormInterface and #1913856: Convert Views UI forms to use FormInterface are done.

However, I have a good deal of code done for the Views issue, and the pattern I've come to is using service:method when the method needs any other true service injected, and class::method for isolated code (which does happen!).

We should also look into #1801356-55: Entity reference autocomplete using routes, and marking these route controllers as private.

I'm very glad this was opened, as we're just kinda making it up as we go, and it'd be good to have a standard (even if Symfony is divided on this).

dawehner’s picture

If we don't have any big logic in the controllers, beside getting the values and pass it further to the actual object that pulls the data it seems to be okay for me to have a ContainerAware class. Then at least the other object can be UnitTested properly.

Crell’s picture

I'll see what I can research on the Symfony side and report back.

The advise I've been giving to date boils down to "Use a ContainerAware controller during development since you don't know yet what you'll be using, then switch to a service controller when you are done to keep everything tidy and decoupled."

I agree that Controllers should be thin glue code, and therefore unit testing them is not a high priority. However, I'm also aware that "should be thin clue code" is a standard we won't always reach, especially in contrib, so I fully expect we will still have a fair number of "real code-carrying" controllers in practice.

dawehner’s picture

I'm wondering whether it's okay to go both with of them and tell people what's the recommended way, and when
you probably can't avoid to use a containeraware controller.

msonnabaum’s picture

It would be unfortunate to have to register each controller as a service whether it needs services injected or not.

It would also be unfortunate to use ContainerAware by default because it hides dependencies and gives the controller class access to way more than it needs.

I'd just solve this with a factory method.


class ControllerBase {
  public static function create() {
    return new static();
  }
}

class ViewsUIController extends ControllerBase {
  public static function create() {
    // Replaced with the static method version once that goes in.
    $container = drupal_container();
    return new static(
      $container->get('plugin.manager.entity'),
      $container->get('views.views_data'),
      $container->get('user.tempstore')
    );
  }
}

And then this small change in ControllerResolver::createController():

 -      $controller = new $class();
 +      $controller = $class::create();

All dependencies are clear, classes remain unit testable, and we avoid registering a ton of services that aren't really services.

Crell’s picture

I talked with some Symfonians about how to benchmark the DIC's scaling potential. When I have a chance to do so I'll post my results.

#5 is interesting, but if we do that we would want to use some other notation for it than reusing the existing notation in ControllerResolver. Changing the meaning of Class::method would be an inconsistency with Symfony, and we don't want to build in land-mines that make code sharing harder than it needs to be. (Fabien is already trying to port some Symfony tools to Drupal, which is awesome. Let's not make that harder than it needs to be.)

tim.plunkett’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
11.81 KB

Here is @msonnabaum's idea as a patch.
It's nicer DX in that you define the dependencies inside the class, no more need for bundles.
The only drawback is finding out about the magic create() method in the first place, though the same could be said for the 'service:method' notation.

Also, we need a nice boilerplate docblock pattern to use on this new create() method.

If this will be the new Best Practice for this (which I think is the goal of this issue), we should do this soon before more stuff gets converted. Bumping to major, though I'm tempted to call it critical, since the inconsistencies in core are really the most confusing part.

Status: Needs review » Needs work

The last submitted patch, route-controllers-1915774-7.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up, +Dependency Injection (DI)
+++ b/core/lib/Drupal/Core/ControllerResolver.php
@@ -74,7 +74,14 @@ protected function createController($controller) {
-      $controller = new $class();
+      // If the controller has a factory method named 'create', pass the
+      // container to it in order to instantiate the class.
+      if (method_exists($class, 'create')) {
+        $controller = $class::create($this->container);
+      }
+      else {
+        $controller = new $class();
+      }
       if ($controller instanceof ContainerAwareInterface) {
         $controller->setContainer($this->container);
       }

Hm. That rather looks like a new ContainerAwareControllerInterface to me?

The subsequent if condition should probably be changed into an elseif, too?

+++ b/core/modules/user/lib/Drupal/user/UserBundle.php
@@ -27,8 +27,6 @@ public function build(ContainerBuilder $container) {
-    $container->register('user.autocomplete_controller', 'Drupal\user\UserAutocompleteController')
-      ->addArgument(new Reference('user.autocomplete'));

+++ b/core/modules/user/user.routing.yml
@@ -8,13 +8,13 @@ user_register:
-    _controller: 'user.autocomplete_controller:autocompleteUser'
+    _controller: '\Drupal\user\UserAutocompleteController::autocompleteUser'

I followed past comments via mail. While it looks nice that we can skip registering controllers as services, this will inherently hinder swappability of controllers, since they're hard-coded all over the place, no?

If we expect that most controllers will consist of relatively dumb glue code only, I wonder why I need to write a controller class in the first place?

tim.plunkett’s picture

FileSize
1.52 KB
14.76 KB

I'm not sure that we need a new interface, but that's a possibility.
And no, technically something that uses create() could still implement ContainerAwareInterface... So I'm leaving the logic as is for now.

Routes themselves can still be switched out, I don't think we're really limiting ourselves that much.

Fixed the tests.

catch’s picture

Priority: Major » Critical

This looks better to me.

I don't really want to commit any more conversions until this is resolved, so bumping to critical per #7.

Like Tim said given routes themselves can be changed around however I don't think this removes any flexibility.

Crell’s picture

OK, I have some benchmarks for the impact of different container sizes.

I setup a simple module that just spools a trivial service into the DIC inside a loop, to fill it up. Then I banged on it with ab, count of 500. Here's what I found (number of dummy services, size of file on disk, and mean response time according to ab):

1:     182192 bytes, 67 ms mean
10:    186017 bytes, 66 ms mean
100:   224537 bytes, 65 ms mean
1000:  612437 bytes, 64 ms mean
2000: 1046437 bytes, 65 ms mean
2500: 1263437 bytes, 90 ms mean
3000: 1480437 bytes, 97 ms mean
4000: 1914437 bytes, 108 ms mean
5000: 2348437 bytes, 115 ms mean

Basically flat until 2000-2500 entries, after which it starts to go up. So what happens there?

With help from Mark Sonnabaum in IRC, we tracked it down to the apc.max_file_size. That defaults to 1 MB, after which it just doesn't bother caching a file. 1 MB is right about the 2000-item mark in these tests. I cranked it up to to 5 MB, then set the number of dummy services to 10,000:

10000: 4518437 bytes, 69 ms mean

The entire file fits inside APC, so the time is still nearly flat. So basically, there is no limit on DIC size other than what fits in APC, at least as far as CPU is concerned.

According to xhprof, the memory usage of the 10k run is about 4.1 MB just for the container. That's close to the size of the file on disk. I didn't check xhprof at all sizes, but it looks like the memory usage is going to scale roughly linearly, as the file gets bigger its memory usage increases about the same. (I'm assuming that the file size impact of a service is roughly constant; that's true for a simple service, but one with lots of dependencies would be slightly larger. Still, that's a decent approximation.)

All tests were conducted on PHP 5.4.6. Mark noted that we should try the tests on PHP 5.3, too, as 5.4 has a number of memory optimizations in it. I've sent him my code to test with, since I don't have a 5.3 box at home anymore, just my 5.4 laptop.

Note: I know that says nothing about the DX question, but the performance question is also relevant. At the moment, it looks like a large container has a linear memory impact, and only a CPU impact if it doesn't fit into APC but that's tweakable. The default APC configuration starts to show strain around 2000 items, give or take.

msonnabaum’s picture

Here are my results on 5.3, parse from 50 xhprof samples each:

+-------------------+------------+-------------+------------+------------+------------+
| namespace         |        min |         max |       mean |     median |       95th |
+-------------------+------------+-------------+------------+------------+------------+
| Calls             |            |             |            |            |            |
|                   |            |             |            |            |            |
| dic1              |     17,062 |     143,284 |     19,586 |     17,062 |     17,062 |
| dic10             |     17,062 |     146,704 |     19,655 |     17,062 |     17,062 |
| dic100            |     17,062 |     180,904 |     20,339 |     17,062 |     17,062 |
| dic1000           |     17,062 |     522,904 |     27,179 |     17,062 |     17,062 |
| dic10000          |     17,062 |   3,942,904 |     95,579 |     17,062 |     17,062 |
|                   |            |             |            |            |            |
| Wall time         |            |             |            |            |            |
|                   |            |             |            |            |            |
| dic1              |    123,585 |     737,134 |    140,420 |    127,464 |    134,477 |
| dic10             |    123,129 |     738,110 |    140,738 |    127,338 |    137,273 |
| dic100            |    124,841 |     921,167 |    144,708 |    127,479 |    136,793 |
| dic1000           |    123,084 |   2,814,406 |    183,524 |    128,215 |    142,376 |
| dic10000          |    129,799 |  21,921,408 |    588,309 |    145,912 |    217,268 |
|                   |            |             |            |            |            |
| Memory usage      |            |             |            |            |            |
|                   |            |             |            |            |            |
| dic1              |  8,331,544 |  12,100,680 |  8,591,364 |  8,331,544 |  9,100,032 |
| dic10             |  8,335,056 |  12,208,552 |  8,571,190 |  8,335,056 |  9,128,376 |
| dic100            |  8,372,608 |  13,288,568 |  8,575,257 |  8,372,608 |  9,415,904 |
| dic1000           |  8,737,544 |  19,218,904 |  9,445,058 |  8,737,544 | 12,293,880 |
| dic10000          | 12,452,528 |  88,418,512 | 16,847,553 | 12,452,528 | 41,209,568 |
|                   |            |             |            |            |            |
| Peak memory usage |            |             |            |            |            |
|                   |            |             |            |            |            |
| dic1              |  8,409,352 |  12,194,392 |  8,669,490 |  8,409,352 |  9,177,840 |
| dic10             |  8,413,024 |  12,302,272 |  8,649,441 |  8,413,024 |  9,206,184 |
| dic100            |  8,450,592 |  13,382,032 |  8,653,550 |  8,450,592 |  9,493,888 |
| dic1000           |  8,815,360 |  19,325,840 |  9,523,484 |  8,815,360 | 12,371,888 |
| dic10000          | 12,530,248 | 101,359,736 | 18,134,731 | 12,530,248 | 50,809,176 |
|                   |            |             |            |            |            |
+-------------------+------------+-------------+------------+------------+------------+

We've made a bigger deal about smaller memory increases than this.

Also, although the performance results aren't great, I dont think this is a performance issue, it's more of a DX/design issue. We shouldn't put controllers in the container because they aren't services.

sdboyer’s picture

as a general rule, i agree that most controllers probably don't belong in the DIC. it certainly doesn't seem like a policy worth adopting, at least - most controllers should, ideally, be pretty thin.

however, i should note that the current drunk controller patch DOES register it as a service. i've done that because it actually does smell a little more like a service:

  • we intend it to conform to an interface and be swappable, as there are a number of use cases for plugging in other controllers. swapping it out by just replacing the named service in the DIC is a nice, easy way to do that globally.
  • it actually DOES need several services to do its thing.

dammit, i forgot the third bullet, oh well. this may matter less than with these other controllers, though, as this will never be written directly into core YAML files, but injected during route altering.

also, wrt the number of controllers: i don't really see a case where we have thousands of controllers; just this one controller should be used for pretty much all HTML routes.

tim.plunkett’s picture

I think we can all agree that some controllers should be services, and we won't have a hard rule.

But we do need an approach for everything else in hook_menu, especially for the forms (with FormInterface).

I'd like some further discussion about the approach taken in #10, any thoughts on that change?

Crell’s picture

After discussion this morning in the WSCCI meeting and some follow-up, I think #5/#10 is probably our best way forward as a "default" position. There will be cases where we'd want to make a controller a for-reals service (most notably wrapping controllers, such as what Sam's working on) and some may need to be ContainerAware because the services they need are highly variable, but I think the factory method will work as the typical case. It still is *mostly* decoupled, in theory allows a controller to be unit tested, documents the needed dependencies, and does so in one location. Annotations would as well, but doing it this way has the added advantage of allowing minimal processing, e.g. pulling values out of the config system and passing just the result to the constructor rather than the entire config service. That sits well with me.

My only pushback would be that we probably should standardize the name of the factory method to avoid confusion, which in turn necessitates some other syntax than 3-part-colon to indicate to use this approach. That probably does imply an interface. If we can then figure out a syntax to support that, we can move forward here quickly. (I'd like to get something landed before sprint weekend if at all possible, which I know is cutting it quite close.)

Crell’s picture

Or, duh, we can just reuse the same syntax safely. If a classname controller implements ControllerInterface, instantiate it using a factory method defined by that interface. If not, just use an empty constructor as before. No new syntax needed.

The Views changes from #10 didn't apply for some reason, so rather than figure out why I just left them out. We can convert that later since the service-style should still work exactly as before. This just converts user autocomplete.

msonnabaum’s picture

Very good solution. Better to enforce at the Interface level.

Status: Needs review » Needs work

The last submitted patch, 1915774-controller-base.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.1 KB
16.24 KB

You forgot to add the new file that defines ControllerInterface :P

I assume it looked something like this, most probably with better docs. Also re-created the Views UI changes.

dawehner’s picture

FileSize
905 bytes
16.27 KB

Let's improve the documentation of that resolver a bit more.

Everything else looks really great. I was a bit unsure about the empty line in the use statements, but yeah that doesn't really matter.

Crell’s picture

Crell--

Yeah, I had a a nice descriptive docblock on the method, but it's on my laptop at home. Let me try and remember what I had and we can land this fast:

/**
 * Instantiates a new instance of this controller.
 * 
 * This is a factory method that returns a new instance of this object. The factory should
 * pass any needed dependencies into the constructor of the this object, but not the container
 * itself. Every call to this method must return a new instance of this object; that is, it may
 * not implement a singleton.
 *
 * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
 *   The service container this object should use.
 */
public static function create(ContainerInterface $container);

(That's not quite what I had before, but I think it hits the same points.)

amateescu’s picture

FileSize
862 bytes
16.6 KB

I think that's good enough for now, we can always tweak it in a followup.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That comment is great, as it also mention to not put the container in there directly.

catch’s picture

Title: Decide whether core route controllers should generally/always be DIC services or not » Change notice: Decide whether core route controllers should generally/always be DIC services or not
Status: Reviewed & tested by the community » Active
+++ b/core/lib/Drupal/Core/ControllerResolver.phpundefined
@@ -66,28 +66,35 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+    // Controller in the service:method notation.
+    $count = substr_count($controller, ':');
+    if ($count == 1) {
+      list($service, $method) = explode(':', $controller, 2);
+      return array($this->container->get($service), $method);
+    }
+
+    // Controller in the class::method notation.
     if (strpos($controller, '::') !== FALSE) {
       list($class, $method) = explode('::', $controller, 2);
-
       if (!class_exists($class)) {
         throw new \InvalidArgumentException(sprintf('Class "%s" does not exist.', $class));

Wonder a bit why we don't do strpos twice - once for '::' then once for ':'. Just taste though so followup if at all.

+++ b/core/modules/user/lib/Drupal/user/UserBundle.phpundefined
@@ -27,8 +27,6 @@ public function build(ContainerBuilder $container) {
-    $container->register('user.autocomplete_controller', 'Drupal\user\UserAutocompleteController')
-      ->addArgument(new Reference('user.autocomplete'));

Yay.

+++ b/core/modules/user/user.routing.ymlundefined
@@ -8,13 +8,13 @@ user_register:
-    _controller: 'user.autocomplete_controller:autocompleteUser'

Also yay.

Committed/pushed this one. Leaving open since this'll need to be updated in change notice somewhere.

jibran’s picture

Issue tags: +Needs change record

Taging

Crell’s picture

Title: Change notice: Decide whether core route controllers should generally/always be DIC services or not » Decide whether core route controllers should generally/always be DIC services or not
Status: Active » Fixed

I think this was covered by http://drupal.org/node/1800686

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

drupal dot orrrrrrrrggggggg

xjm’s picture

h8