Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
If we take a look at http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul..., we can see that 90 LoC are needed for a method that is only 5 lines. I call that madness :)
Proposed resolution
Introduce a ControllerBase class that is ContainerAware and has the same helper methods as \Drupal.
Remaining tasks
Expand and properly word the definition of 'thin' controllers.
User interface changes
None.
API changes
None, just an API addition.
Followups
#2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface
Comment | File | Size | Author |
---|---|---|---|
#39 | interdiff.txt | 2.8 KB | Crell |
#36 | 2049159-controllerbase.patch | 20.67 KB | Crell |
#12 | 2049159-controllerbase.patch | 21.07 KB | Crell |
#12 | interdiff.txt | 3.67 KB | Crell |
#7 | 2049159-7.patch | 18.68 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedHere it is, with TaxonomyController converted to use it.
Comment #2
dawehnerThis is a huge WIN for dx, especially in custom world, when you simply won't write unit tests anyway.
Maybe this also encourage people to write actually thin controllers. This is a general problem we have to document.
One problem which comes with that patch is that people start to rework existing conversions, which takes away some effort on solving other actual problems.
The module handler should use the module handler interface.
I am a bit confused that the typed data manager is one there, as it does not seem to be an often needed service?
The url generator should also use the Path thingy generator interface
Comment #3
amateescu CreditAttribution: amateescu commentedThanks for reviewing! Here's the two updates needed, but I'm afraid you'll have to take the point about typed data manager to the guys who put it in \Drupal, because I only want to have a 1:1 mapping here.
Comment #4
amateescu CreditAttribution: amateescu commented@dawehner, your instinct was correct :) After some more discussions yesterday night in IRC, it turns out that we don't actually want a 1:1 mapping to \Drupal, but only include methods that make sense in a Controller context. The database service was the first one voted to go out, but after looking at our existing controllers, I also took the liberty to remove entityQuery(), entityQueryAggregate(), flood(), typedData() and token().
typedData() wasn't really used anywhere and the others had one or very few usages We could argue that all the other 4 that were removed might make more sense in a FormControllerBase perhaps, since form controllers usually have more work to do than the dumb ones.
Oh, and I also converted a few more controllers, just to show how much boilerplate code we can get rid of.
Comment #5
catchReally like the look of this.
Comment #6
Crell CreditAttribution: Crell commentedI question if this is appropriate for well-behaved controllers. Locking should happen further down within a service, no?
Is this common enough for a thin controller to care? Seems like this should mostly be used by Actions...
Quite rare.
In for a penny, in for a pound...
public function translation($str, $arr) {
return $this->container->get('string_translation')->translate($str, $arr);
}
?
I think the guideline for what methods to offer in the base class should not be "what are controllers commonly using" but "what do we want controllers to commonly use". That uses it as a guide for "if it's clunky to do from a controller, you probably shouldn't do it in a controller."
Comment #7
amateescu CreditAttribution: amateescu commentedYeah, I guess I should've been less merciful with what methods to keep. Removed lock(), queue() and httpClient() and changed translation() to be the equivalent of t().
@Crell, would you like to take a stab at the doxygen of the class? I know you like these things :)
Comment #8
Crell CreditAttribution: Crell commentedI'll try to have a crack at it shortly. :-)
Comment #9
alexpottMaybe we could have a redirect method too and solve the redirect DX issue as this is quite a common task from a controller...
Comment #10
amateescu CreditAttribution: amateescu commentedI don't have anything against that. Maybe it makes sense to add it to \Drupal as well?
But can we please name it drupalGoto()? :D #trololol
Comment #11
dawehnerJust wondering about how we can encourage people to use the url generator instead of the old path based methods. As crell said, this helpers are about showing proper standards, using route name + parameters would be maybe better?
Comment #12
Crell CreditAttribution: Crell commentedHere we are. I added descriptions to both ControllerBase and ControllerInterface about what they're for and when you'd use one or the other, and why using ControllerBase with a thin controller is better.
I also fixed a copy/paste issue in another docblock, and added a route-based redirect method.
For reference, here's what Symfony fullstack's base controller class looks like:
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/Framew...
Perhaps we should add a generateUrl() as well? Not sure, but we can always tweak the list here. (And it's easier to add than to remove, so maybe we should keep the list short for now.)
Comment #14
plach#13 is weird ;)
Comment #15
amateescu CreditAttribution: amateescu commentedIt was just a random fail so I asked for a retest on qa.d.o :)
Comment #16
amateescu CreditAttribution: amateescu commentedTag monster--
Comment #17
dawehnerI would kind of disagree, it just makes it way harder. The word "renders" make it really hard to understand for non-native english but drupal-native speaking people.
Missing empty line between @param and @return
Comment #18
chx CreditAttribution: chx commentedEh. Why the heck are those not unit testable? Nonsense. Create an appropriate mock container and inject it.
Comment #19
xjmSo, this patch is FTW. (Edit: For the non-gamers, that means "for the win"; it's not just WTF backwards.) ;) I wonder though if we need better names? Doesn't have to happen in this issue, but having a ControllerBase that does NOT implement ControllerInterface is weird, and ControllerInterface seems like it could use a better name anyway (as @webchick pointed out in another discussion).
Comment #20
xjmAlso, is our expectation that all controllers should either extend
ControllerThingBase
or implementControllerThingInterface
? If so let's state that explicitly. And that's still a weird pattern. MaybeProperlyInjectableControllerInterface extends ControllerInterface
is what addscreate()
, and the (so far empty)ControllerInterface
is implemented byControllerBase
? Or am I overthinking it?Comment #21
amateescu CreditAttribution: amateescu commentedYou are definitely overthinking it :P
Yep, this is correct and it's what we need to document properly.
Comment #22
dawehnerTechnical seen this is not really true, but in reality this is the case in most of the cases.
Maybe out of scope, but I don't care.
-1 for this change, because PathBasedgeneratorInterface does not allow you to use ->generate. #1965074: Add cache wrapper to the UrlGenerator solves that.
Comment #23
amateescu CreditAttribution: amateescu commentedI agree about the technical part not being true, but let's be honest, why would you implement ControllerInterface if you already have the container available..
I also don't care about the changes in Drupal.php. I did them initially when I wanted to keep the helper methods in sync, but now I guess we can just open another issue for that part of the patch.
So.. either I found the wrong interface that you were hinting at, or you need to make up your mind regarding this sentence from #2 :)
Comment #24
dawehnerLet's just open a parallel issue: #2057215: PathBasedGeneratorInterface should extend the base ones, so the proper generate method can also be used
Comment #25
xjmMaybe it would make more sense for
ControllerBase
to have a more specific name, implementControllerInterface
, and implementcreate()
on behalf of its descendants?Comment #26
amateescu CreditAttribution: amateescu commented@xjm, please see #21 and my first paragraph from #23. There is no reason for ControllerBase to interact with ControllerInterface in any way...
@dawehner, so I did find the correct interface that you wanted in #2 but it turns out it's actually not ok to use it? Can we then just go back to
\Drupal\Core\Routing\UrlGenerator
and let the switch to an interface happen in #2057215: PathBasedGeneratorInterface should extend the base ones, so the proper generate method can also be used?@Crell, do you think we can incorporate the feedback from @chx in #18?
Comment #27
tim.plunkett@xjm, ControllerInterface is the one that is misnamed here. ControllerInjectionInterface would be more appropriate.
Comment #28
webchick+1 for ControllerInjectionInterface. Could we spin off a sub-issue for that?
Comment #29
xjm@amateescu, I read your comments, and disagreed with them. Having FooInterface and FooBase and saying they don't interact in anyway is either bad naming, bad architecture, or both.
@tim.plunkett and @webchick, that was my other suggestion above in #20, and @amateescu told me I was an idiot.
Comment #30
amateescu CreditAttribution: amateescu commentedI did what?
Comment #31
amateescu CreditAttribution: amateescu commentedI fully agree with this, my previous comments about one not having anything to do with the other were about our *current code*, not theoretically. Yes, in this case it's about bad naming.
That is not what you suggested. Tim's suggestion is to rename ControllerInterface, not to create another interface that extends it.
Comment #32
chx CreditAttribution: chx commented@xjm I read this issue and nowhere did @amateescu call you an idiot. All he said about you was "You are definitely overthinking it :P" which is a very far cry.
Please, everyone keep to technical arguments and leave the personal issues out of the issue queue. Thanks for keeping the kettle black,
pot.
Comment #33
amateescu CreditAttribution: amateescu commentedActually, to hell with this issue, unfollowing.
Comment #34
tim.plunkettHonestly.
Comment #35
msonnabaum CreditAttribution: msonnabaum commentedYes, ControllerBase is a perfectly fine name. Let's keep that and change ControllerInterface.
Comment #36
Crell CreditAttribution: Crell commentedRevised patch.
1) Softens the language about testability a bit, while keeps it clear that ControllerBase and good testing are not friends.
2) Removes keyvalueExpireable(), since that is really just vestigial from the original "everything from \Drupal" version.
3) On the name, IMO changing ControllerInterface is a major API break at this point, and I am not willing to burn any karma on it. I have bigger things to burn karma on. :-) If someone else wants to charge up that hill they're welcome to do so but I won't. (If that happens, CreateableInterface seems most descriptive since it's not strictly speaking Controller-centric behavior; it's just where we're using it.)
If the bot approves IMO this is commitable. But FTLOD let's not bikeshed this issue on class names.
Comment #37
xjmPoor choice of words. My apologies.
Fortunately, at least one core maintainer wants to see the name changed, so as long as you're not opposed to the rename, you don't have to. ;)
Comment #38
xjm@Crell, do you have an interdiff for #36?
Comment #39
Crell CreditAttribution: Crell commentedNot much to it, but here.
Comment #40
Crell CreditAttribution: Crell commentedWell, if we're going to change it let's try to change it in the next week or so, before MWDS and the training we're running there. :-)
Comment #41
tim.plunkettThanks, the new docs are much better, and you're right about probably not needing that method.
Comment #42
alexpottFollow-up created #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface
Comment #43
alexpottCommitted 7412e6f and pushed to 8.x. Thanks!
Comment #44
Crell CreditAttribution: Crell commentedThis should probably just be added to an existing change notice, not a new one. Unless we want the Twitter notification for existing devs?
Comment #45
webchickThis patch makes me so happy my insides are glowing! Thanks to all who worked on it. :D
Comment #46
tim.plunkettSee #2059245: Add a FormBase class containing useful methods for a similar issue for FormInterface
Comment #47
tim.plunkettThe followup was created. Still needs a change notice.
Comment #48
yched CreditAttribution: yched commentedNice addition indeed :-)
Question: Symfony fullstack's base controller (linked by @Crell in #12) has a get() method as a shortcut for $this->container->get($id);
Wouldn't it be worth having in there too ? I mean, the drawbacks about unit testability and writing thick controllers with this are in the class doc ?
Comment #49
tim.plunkettYou ideally wouldn't use anything not already a protected method. If you need more, don't use ControllerBase.
So we don't want to make it easier to abuse.
Comment #50
smiletrl CreditAttribution: smiletrl commentedAdd a change record here https://drupal.org/node/2060189.
Comment #51
YesCT CreditAttribution: YesCT commentedneeds review to review the change record.
Comment #52
dawehnerIt is odd to link to a non-committed patch in the change notice, otherwise it is looking great, maybe a code example would help.
Comment #53
smiletrl CreditAttribution: smiletrl commentedAdd code example there.
Yeah, it still needs tweaking, when we settle #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface.
Comment #54
smiletrl CreditAttribution: smiletrl commentedinterface ContainerInjectionInterface is in. Fixed the change record.
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedRestoring stuff
Comment #55.0
ParisLiakos CreditAttribution: ParisLiakos commentedUpdated issue summary.