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.
Comment | File | Size | Author |
---|---|---|---|
#23 | drupal-1915774-23.patch | 16.6 KB | amateescu |
#23 | interdiff.txt | 862 bytes | amateescu |
#21 | drupal-1915774-21.patch | 16.27 KB | dawehner |
#21 | interdiff.txt | 905 bytes | dawehner |
#20 | 1915774-controller-base-20.patch | 16.24 KB | amateescu |
Comments
Comment #1
tim.plunkettThe 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).
Comment #2
dawehnerIf 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.
Comment #3
Crell CreditAttribution: Crell commentedI'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.
Comment #4
dawehnerI'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.
Comment #5
msonnabaum CreditAttribution: msonnabaum commentedIt 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.
And then this small change in ControllerResolver::createController():
All dependencies are clear, classes remain unit testable, and we avoid registering a ton of services that aren't really services.
Comment #6
Crell CreditAttribution: Crell commentedI 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.)
Comment #7
tim.plunkettHere 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.
Comment #9
sunHm. That rather looks like a new
ContainerAwareControllerInterface
to me?The subsequent if condition should probably be changed into an elseif, too?
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?
Comment #10
tim.plunkettI'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.
Comment #11
catchThis 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.
Comment #12
Crell CreditAttribution: Crell commentedOK, 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):
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.
Comment #13
msonnabaum CreditAttribution: msonnabaum commentedHere are my results on 5.3, parse from 50 xhprof samples each:
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.
Comment #14
sdboyer CreditAttribution: sdboyer commentedas 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:
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.
Comment #15
tim.plunkettI 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?
Comment #16
Crell CreditAttribution: Crell commentedAfter 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.)
Comment #17
Crell CreditAttribution: Crell commentedOr, 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.
Comment #18
msonnabaum CreditAttribution: msonnabaum commentedVery good solution. Better to enforce at the Interface level.
Comment #20
amateescu CreditAttribution: amateescu commentedYou forgot to add the new file that defines
ControllerInterface
:PI assume it looked something like this, most probably with better docs. Also re-created the Views UI changes.
Comment #21
dawehnerLet'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.
Comment #22
Crell CreditAttribution: Crell commentedCrell--
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:
(That's not quite what I had before, but I think it hits the same points.)
Comment #23
amateescu CreditAttribution: amateescu commentedI think that's good enough for now, we can always tweak it in a followup.
Comment #24
dawehnerThat comment is great, as it also mention to not put the container in there directly.
Comment #25
catchWonder a bit why we don't do strpos twice - once for '::' then once for ':'. Just taste though so followup if at all.
Yay.
Also yay.
Committed/pushed this one. Leaving open since this'll need to be updated in change notice somewhere.
Comment #26
jibranTaging
Comment #27
Crell CreditAttribution: Crell commentedI think this was covered by http://drupal.org/node/1800686
Comment #29
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #30
xjmdrupal dot orrrrrrrrggggggg
Comment #31
xjmh8