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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
12.49 KB

Here it is, with TaxonomyController converted to use it.

dawehner’s picture

This 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

amateescu’s picture

FileSize
1.58 KB
13.12 KB

Thanks 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.

amateescu’s picture

FileSize
12 KB
19.46 KB

@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.

catch’s picture

Really like the look of this.

Crell’s picture

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -0,0 +1,187 @@
+  /**
+   * Returns the locking layer instance.
+   *
+   * @return \Drupal\Core\Lock\LockBackendInterface
+   */
+  protected function lock() {
+    return $this->container->get('lock');
+  }
+

I question if this is appropriate for well-behaved controllers. Locking should happen further down within a service, no?

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -0,0 +1,187 @@
+  protected function queue($name, $reliable = FALSE) {
+    return $this->container->get('queue')->get($name, $reliable);

Is this common enough for a thin controller to care? Seems like this should mostly be used by Actions...

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -0,0 +1,187 @@
+  protected function httpClient() {
+    return $this->container->get('http_default_client');

Quite rare.

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -0,0 +1,187 @@
+  /**
+   * Returns the string translation service.
+   *
+   * @return \Drupal\Core\StringTranslation\TranslationManager
+   *   The string translation manager.
+   */
+  protected function translation() {
+    return $this->container->get('string_translation');
+  }

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."

amateescu’s picture

Component: routing system » base system
FileSize
3.56 KB
18.68 KB

Yeah, 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 :)

Crell’s picture

Assigned: amateescu » Crell

I'll try to have a crack at it shortly. :-)

alexpott’s picture

Maybe we could have a redirect method too and solve the redirect DX issue as this is quite a common task from a controller...

  protected function redirect($url) {
    return new RedirectResponse(
      $this->container->get('url_generator')->generateFromPath($url, array('absolute' => TRUE))
    );
  }
amateescu’s picture

I 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

dawehner’s picture

Just 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?

Crell’s picture

Here 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.)

Status: Needs review » Needs work

The last submitted patch, 2049159-controllerbase.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#13 is weird ;)

amateescu’s picture

Issue tags: -WSCCI

It was just a random fail so I asked for a retest on qa.d.o :)

amateescu’s picture

Issue tags: +WSCCI

Tag monster--

dawehner’s picture

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.phpundefined
@@ -8,12 +8,26 @@
+ * handling code.  However, it also renders the class impossible to unit test.

I 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.

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.phpundefined
@@ -152,4 +166,20 @@ protected function languageManager() {
+   * @param int $status
+   *   The HTTP redirect status code for the redirect. The default is 302 Found.
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse

Missing empty line between @param and @return

chx’s picture

Eh. Why the heck are those not unit testable? Nonsense. Create an appropriate mock container and inject it.

xjm’s picture

So, 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).

xjm’s picture

Also, is our expectation that all controllers should either extend ControllerThingBase or implement ControllerThingInterface? If so let's state that explicitly. And that's still a weird pattern. Maybe ProperlyInjectableControllerInterface extends ControllerInterface is what adds create(), and the (so far empty) ControllerInterface is implemented by ControllerBase? Or am I overthinking it?

amateescu’s picture

You are definitely overthinking it :P

Also, is our expectation that all controllers should either extend ControllerThingBase or implement ControllerThingInterface?

Yep, this is correct and it's what we need to document properly.

dawehner’s picture

Yep, this is correct and it's what we need to document properly.

Technical seen this is not really true, but in reality this is the case in most of the cases.

index db2552b..672a0d1 100644
--- a/core/lib/Drupal.php

Maybe out of scope, but I don't care.

+++ b/core/lib/Drupal.phpundefined
@@ -364,7 +364,7 @@ public static function token() {
-   * @return \Drupal\Core\Routing\UrlGenerator
+   * @return \Drupal\Core\Routing\PathBasedGeneratorInterface

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.phpundefined
@@ -0,0 +1,185 @@
+   * @return \Drupal\Core\Routing\PathBasedGeneratorInterface

-1 for this change, because PathBasedgeneratorInterface does not allow you to use ->generate. #1965074: Add cache wrapper to the UrlGenerator solves that.

amateescu’s picture

Technical seen this is not really true, but in reality this is the case in most of the cases.

I 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.

-1 for this change, because PathBasedgeneratorInterface does not allow you to use ->generate.

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 :)

The url generator should also use the Path thingy generator interface

dawehner’s picture

xjm’s picture

Maybe it would make more sense for ControllerBase to have a more specific name, implement ControllerInterface, and implement create() on behalf of its descendants?

amateescu’s picture

@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?

tim.plunkett’s picture

@xjm, ControllerInterface is the one that is misnamed here. ControllerInjectionInterface would be more appropriate.

webchick’s picture

+1 for ControllerInjectionInterface. Could we spin off a sub-issue for that?

xjm’s picture

@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.

amateescu’s picture

I did what?

amateescu’s picture

Having FooInterface and FooBase and saying they don't interact in anyway is either bad naming, bad architecture, or both.

I 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.

@tim.plunkett and @webchick, that was my other suggestion above in #20, and @amateescu told me I was an idiot.

That is not what you suggested. Tim's suggestion is to rename ControllerInterface, not to create another interface that extends it.

chx’s picture

@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.

amateescu’s picture

Actually, to hell with this issue, unfollowing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

Honestly.

msonnabaum’s picture

Yes, ControllerBase is a perfectly fine name. Let's keep that and change ControllerInterface.

Crell’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
20.67 KB

Revised 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.

xjm’s picture

Poor choice of words. My apologies.

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. :-)

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. ;)

xjm’s picture

@Crell, do you have an interdiff for #36?

Crell’s picture

FileSize
2.8 KB

Not much to it, but here.

Crell’s picture

Well, 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. :-)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, the new docs are much better, and you're right about probably not needing that method.

alexpott’s picture

alexpott’s picture

Title: Create a ControllerBase class to stop the boilerplate code madness » [Change notice] Create a ControllerBase class to stop the boilerplate code madness
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 7412e6f and pushed to 8.x. Thanks!

Crell’s picture

This should probably just be added to an existing change notice, not a new one. Unless we want the Twitter notification for existing devs?

webchick’s picture

This patch makes me so happy my insides are glowing! Thanks to all who worked on it. :D

tim.plunkett’s picture

See #2059245: Add a FormBase class containing useful methods for a similar issue for FormInterface

tim.plunkett’s picture

The followup was created. Still needs a change notice.

yched’s picture

Nice 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 ?

tim.plunkett’s picture

You 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.

smiletrl’s picture

Add a change record here https://drupal.org/node/2060189.

YesCT’s picture

Status: Active » Needs review

needs review to review the change record.

dawehner’s picture

It is odd to link to a non-committed patch in the change notice, otherwise it is looking great, maybe a code example would help.

smiletrl’s picture

Add code example there.

Yeah, it still needs tweaking, when we settle #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface.

smiletrl’s picture

Status: Needs review » Closed (fixed)

interface ContainerInjectionInterface is in. Fixed the change record.

ParisLiakos’s picture

Title: [Change notice] Create a ControllerBase class to stop the boilerplate code madness » Create a ControllerBase class to stop the boilerplate code madness
Priority: Critical » Major
Issue tags: -Needs change record

Restoring stuff

ParisLiakos’s picture

Issue summary: View changes

Updated issue summary.