ControllerInterface is the misnamed here and clashes with ControllerBase and controllers can be controllers without implementing it. ControllerInjectionInterface would be more appropriate. See #2049159: Create a ControllerBase class to stop the boilerplate code madness

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

The -do-not-test patch contains only the changes for this issue.

The patch that is tested is rolled on top of #2049159: Create a ControllerBase class to stop the boilerplate code madness

Crell’s picture

s/ControllerInjectionInterface/CreateableControllerInterface/ :-)

xjm’s picture

xjm’s picture

Issue tags: +API change
webchick’s picture

Priority: Normal » Major

Here's my stance for why we should allow this API change post-API freeze, and why this is "major" rather than "normal". Because this is quite a developer-facing thing and will break modules that are already in the process of being ported, I'd love a second opinion from another core maintainer (unless I should interpret the fact that alexpott spun off this issue to be the second opinion in favour of this change :)).

I came back from ~3 months away from D8 and started writing https://drupal.org/sandbox/webchick/2051265 because I couldn't stand all the boilerplate code everywhere (+10000 for #2049159: Create a ControllerBase class to stop the boilerplate code madness, incidentally). In order to do so, I needed to copy/paste an existing page controller and try and get the script to output that.

I can't remember which one I copy/pasted from (something in book.module I think), but whichever one it was, it implemented "ControllerInterface." Since this was so generic-sounding, I figured that it was required of controller classes to implement the interface, so I did. I immediately started getting errors about missing "use" statements, so had to add those, and a missing required "create()" method, so I had to add that, too. My module does nothing but spit out a simple "hello world" so I was utterly mystified why I should need to implement that method, since I'm not injecting anything.

That'd be one data point, if it were just me. But then lo and behold Joe Shindelar from Drupalize.me had exactly the same impression, and this is now codified in a tutorial video at http://drupalize.me/blog/201307/drupal-8-writing-hello-world-module.

This made me extremely alarmed about the DX impact we were thrusting on module developers to make even the simplest of pages work. That is, until effulgentsia wrote http://effulgentsia.drupalgardens.com/content/drupal-8-hello-oop-hello-w..., at which point I figured out that you don't have to implement ControllerInterface at all, it's only if you specifically need to inject something that you should do so (and in that case, it makes total sense to force people to have a create() method).

... Long story short, I think that the current naming represents a confusing enough situation DX-wise, especially if #2049159: Create a ControllerBase class to stop the boilerplate code madness makes it in (which I sincerely hope it does), that we should make this change, despite it being post-API freeze, and despite it breaking already-ported modules in D8.

Alex/catch/Dries, do you agree/disagree?

webchick’s picture

Also, FWIW, "CreateableControllerInterface" means absolutely nothing to me, and nothing in here:

 /**
  * Defines a common interface for route controllers.
+ *
+ * This interface gives controller classes a factory method for instantiation
+ * rather than relying on a services.yml entry. However, it may result in
+ * a lot of boilerplate code in the class. As an alternative, controllers that
+ * contain only limited glue code ("thin" controllers) should instead extend
+ * ControllerBase as that allows direct access to the container. That renders
+ * the controller very difficult to unit test so should only be used for
+ * controllers that are trivial in complexity.
  */

...nor here:

  /**
   * 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 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.
   */

...explains it, either.

dawehner’s picture

Yeah creatable controller interface is a horrible name.
What about ContainerControllerInterface, as this is kind of similar to "ContainerFactoryPluginInterface"

alexpott’s picture

Ok renamed it ContainerControllerInterface as this along the lines of the existing ContainerDerivativeInterface and ContainerFactoryPluginInterface which do the same thing for plugins and there derivatives.

Also cleaned up DesignTestController as it was unnecessarily implementing the interface and it was doing it wrong too.

No point in any interdiff to #1 as it'd be the whole patch :)

alexpott’s picture

Title: Rename ControllerInterface to ControllerInjectionInterface » Rename ControllerInterface to ContainerControllerInterface

Updating title to current suggestion

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Eh, not a huge fan of #8 but consistency FTW.

I think this needs a signoff from either catch or Dries (since webchick and Alex are obviously in favor).

webchick’s picture

Assigned: Unassigned » catch

Agreed. Trying catch first. Dries is out until Friday, and if this is going in, the sooner the better, since it very blatantly affects module developers.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Controller/ContainerControllerInterface.phpundefined
@@ -16,11 +16,11 @@
+ * container. That renders the controller very difficult to unit test so should
+ * only be used for controllers that are trivial in complexity.
  */

Trivial in complexity is already here, but that's a horrible way to say 'simple' or 'trivial'?

I'm not at all sure about 'ContainerControllerInterface' - this is specifically not ContainerAware but it sounds very much like that. You don't get access to the container in your controller, which is what the name suggests to me. I agree that 'Createable' doesn't mean much, but didn't mind InjectionControllerInterface or even ContainerInjectionControllerInterface would describe what it does a bit more.

Renaming after code freeze seems OK - worth it to get the other patch in at least, but let's spend a couple more days on the name.

dawehner’s picture

On plugins it is named ContainerFactoryPluginInterface, so we could also go with that: ContainerFactoryControllerInterface

eojthebrave’s picture

I don't have much to add from a technical perspective on this, but from a learnability standpoint I think adding "Container" to the name is helpful. Like webchick, when I went to write a simple hello world module I based it off of other module's already in core since at the moment that's the best documentation available, and was confused about the extra create() method but just rolled with it because it worked.

Changing the name and adding additional documentation to the tune of "implement this interface if you want X, Y, and or Z otherwise use Blah" would be a huge win for the many other people that are likely to encounter this pattern for the first time by just trying to copy/paste the code from a core module and get something working.

+1 :)

msonnabaum’s picture

I'd like to suggest that the word "controller" be removed. This interface has absolutely nothing to do with controllers. All it's saying is, "I'm my own factory and I'd like the container for my dependencies". Nothing about that implies controller.

Best I can come up with atm is ContainerAwareFactory.

smiletrl’s picture

Word "Controller" could imply the routing controllers. But yeah, class implementing this interface is not always called some 'Controller', e.g., ActionAdminManageForm, BanAdmin, etc.

As for 'ContainerAwareFactory', this probably goes back to comment #12. "ContainerAware" could be confusing, even as it mentions the word 'Factory' too:) At this sense, 'ContainerFactoryInterface' seems good to me.

On the other hand, I don't have objection regarding word 'controller'. "ContainerFactoryControllerInterface" is good to me too, since this interface resides in name space "Drupal\Core\Controller":)

dawehner’s picture

Let's also post the other two variants.

Personally I would like ContainerFactoryInterface. The question is whether we can/should move it outside of the Controller directory.

Status: Needs review » Needs work

The last submitted patch, drupal-2057589-controllerfactoryinterface.patch, failed testing.

smiletrl’s picture

There's already a class called **ContainerFactory** As ControllerBase may be confusing with ControllerInterface I guess 'ContainerFactory' will be confusing with 'ContainerFactoryInterface' too.

"ContainerFactoryControllerInterface" seems to be the good choice now.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
60.4 KB

Looks like the first patch in #17 has been ignored by bot. Add it here.

Status: Needs review » Needs work

The last submitted patch, drupal-2057589-controllerfactorycontrollerinterface.patch, failed testing.

xjm’s picture

@smiletrl, I believe that is not the patch being ignored, but rather the patch breaking Drupal. ;) It happened on two different bots. Have you tried running a test locally with the patch applied (or installing Drupal, for that matter?) If not I'd recommend double-checking that those things work.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
60.37 KB

@xjm, yeah, you're right:)

Ok, rerolled the patch at #17 for "ContainerFactoryControllerInterface".

webchick’s picture

Oh, dear. :)

I leave it to someone who understands these things better than I do to RTBC, but at least that's suitably spookier sounding and less likely to be used by newbies who are just learning D8 for the first time, which is what I was going for here.

Status: Needs review » Needs work

The last submitted patch, Rename_controllerInterface-2057589-23.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
61.05 KB

fixed a bug.

Status: Needs review » Needs work

The last submitted patch, Rename_controllerInterface-2057589-26.patch, failed testing.

dawehner’s picture

-    $this->pageController = new ViewPageController($this->container->get('plugin.manager.entity')->getStorageController('view'), new ViewExecutableFactory());
+    $this->pageController = ViewPageController::create($this->container);

I think we tests we should not use the create method, but explicit pass the dependencies.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
61.06 KB

Hmm, not sure about this, is there some preference here, or to make the test more readable -- self-Documented?

I think we tests we should not use the create method, but explicit pass the dependencies.

Fixed more bugs.

Status: Needs review » Needs work

The last submitted patch, Rename_controllerInterface-2057589-29.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
61.09 KB

Rerolled again. Drupal HEAD is going fast:) interdiff is the same with #29.

Status: Needs review » Needs work

The last submitted patch, Rename_controllerInterface-2057589-31.patch, failed testing.

dawehner’s picture

Title: Rename ControllerInterface to ContainerControllerInterface » Rename ControllerInterface to ContainerFactoryControllerInterface

Edit title.

dawehner’s picture

@@ -19,15 +19,15 @@
+ * this base class but use ContainerFactoryControllerInterface instead, or even better
+ * be refactored to be trivial glue code.

> 80 chars.

@@ -6,25 +6,10 @@
-class DesignTestController implements ControllerInterface {
-
-  /**
-   * {@inheritdoc}
-   */
-  public static function create() {
...
-  }
-
-  /**
-   * Constructs a DesignTestController object.
-   */
-  public function __construct() {
-  }

I don't care about this change, but it is a bit out of scope ... at some point there was an active decision to always include the interface, just for consistency

msonnabaum’s picture

I've still not seen a justification for having the world controller in an interface that has nothing to do with controllers.

smiletrl’s picture

Status: Needs work » Needs review

ok, how about 'ContainerInjectionInterface', and put this interface inside name space -- Drupal\Core\DependencyInjection?

'ContainerFactoryInterface' would confuse the existing 'ContainerFactory' class, and word 'Factory' could mean factory class/interface for plugins.

Crell’s picture

I'd be fine with #13, frankly. It's parallel with ContainerFactoryPluginInterface. While Mark's right that there's nothing in the interface that strictly implies it's for Controllers only, well, that's what we're using it for, and the only other place we're using this pattern we specify "Plugin", so...

This is a bikeshed bait issue. Who has the authority to declare that we're going with a given name and have it stick? (If the answer is "no one", then this issue will be won by the most stubborn among us. History has shown that to be a highly inefficient approach to consensus building.)

smiletrl’s picture

Continuous work on ContainerFactoryControllerInterface. Name discussion is still open:)

Status: Needs review » Needs work

The last submitted patch, Rename_controllerInterface-2057589-38.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
623 bytes
63.08 KB

Fix in this patch has passed failed tests in last patch locally. Let's see the online bot.

tim.plunkett’s picture

@smiletrl thanks for getting the patch to pass!

I'd still like to see this have "Injection" in the name, as that's why you'd use it.

smiletrl’s picture

Any ideas about 'ContainerInjectionInterface', and put this interface inside name space -- Drupal\Core\DependencyInjection?

Crell’s picture

If #2076085: Resolve the need for a 'title callback' using the route happens as planned, then that's a non-controller non-plugin class that would also benefit from this interface. So I rescind my call to keep "Controller" in the name.

tim.plunkett’s picture

Title: Rename ControllerInterface to ContainerFactoryControllerInterface » Rename ControllerInterface to ContainerInjectionInterface
FileSize
49.92 KB

+1 to #42.
Rerolled.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -15,7 +15,7 @@
-class EntityViewController implements ControllerInterface {
+class EntityViewController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -22,7 +22,7 @@
  */
-class AggregatorController implements ControllerInterface {
+class AggregatorController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {
 

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php
@@ -8,13 +8,13 @@
-class CustomBlockController implements ControllerInterface {
+class CustomBlockController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -14,7 +14,7 @@
-class BookController implements ControllerInterface {
+class BookController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -23,7 +23,7 @@
-class CommentController implements ControllerInterface {
+class CommentController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -59,10 +59,10 @@ public static function create(ContainerInterface $container) {
-  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage, ControllerInterface $file_download_controller) {
+  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage, \Drupal\Core\DependencyInjection\ContainerInjectionInterface $file_download_controller) {

+++ b/core/modules/menu/lib/Drupal/menu/Controller/MenuController.php
@@ -18,7 +18,7 @@
-class MenuController implements ControllerInterface {
+class MenuController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php
@@ -20,7 +20,7 @@
-class OverlayController implements ControllerInterface {
+class OverlayController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/system/tests/modules/common_test/lib/Drupal/common_test/Controller/CommonTestController.php
@@ -7,13 +7,13 @@
-class CommonTestController implements ControllerInterface {
+class CommonTestController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/system/tests/modules/design_test/lib/Drupal/design_test/Controller/DesignTestController.php
@@ -6,12 +6,12 @@
-class DesignTestController implements ControllerInterface {
+class DesignTestController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
@@ -22,7 +22,7 @@
-class TermAutocompleteController implements ControllerInterface {
+class TermAutocompleteController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Controller/TourTestController.php
@@ -6,13 +6,13 @@
-class TourTestController implements ControllerInterface {
+class TourTestController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/user/lib/Drupal/user/Controller/UserAutocompleteController.php
@@ -10,13 +10,13 @@
-class UserAutocompleteController implements ControllerInterface {
+class UserAutocompleteController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/views/lib/Drupal/views/Controller/ViewAjaxController.php
@@ -21,7 +21,7 @@
-class ViewAjaxController implements ControllerInterface {
+class ViewAjaxController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Controller/ViewsUIController.php
@@ -25,7 +25,7 @@
-class ViewsUIController implements ControllerInterface {
+class ViewsUIController implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface {

/me points on tim for trusting phpstorm, HAHA

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.11 KB
49.31 KB

LOL.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

I think this name is as good as we're going to get.

tim.plunkett’s picture

Rerolled.

catch’s picture

Assigned: catch » Unassigned

I'm happy with this, don't want to commit after only a couple of hours four minutes RTBC, so just posting an unassigning - I'll commit next time I'm looking through queue if no-one beats me to it.

Crell’s picture

I can work with #48. No further objections.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Awesome!! Now we just need a re-roll.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
46.71 KB

Straight reroll.

catch’s picture

Title: Rename ControllerInterface to ContainerInjectionInterface » Change notice: Rename ControllerInterface to ContainerInjectionInterface
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Novice, +Needs change record

Committed/pushed to 8.x, thanks!

yched’s picture

Drupal\Core\DependencyInjection\ContainerInjectionInterface ?
Nothing in there seems to be related to "request controllers". Does it mean it can be used by other stuff than "request controllers" that also have the "create() factory method" pattern ? Plugins, entity controllers... - they're also doing "container injection" ?

smiletrl’s picture

Yes, this name/space change is trying to keep this interface generic, other than tightly coupled to "routing controllers".

Other dependency injected class could implement this interface, as long as it ONLY needs to inject the container services. In some cases, because it requiress more than just a container, this interface isn't suitable then. For instance, EntityControllerInterface (which could be improved at #2015535: Improve instantiation of entity classes and entity controllers) requires two more parameters, including a $container.

Regarding the change record, is it worth another new record, other than mentioning it at https://drupal.org/node/2060189?

tim.plunkett’s picture

They have other required parameters, so they need a different interface.

Crell’s picture

I have updated the WSCCI Conversion Guide here: https://drupal.org/node/1953342

smiletrl’s picture

Status: Active » Needs review
jibran’s picture

Title: Change notice: Rename ControllerInterface to ContainerInjectionInterface » Rename ControllerInterface to ContainerInjectionInterface
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Novice, -Needs change record +Approved API change

Thanks for the change notice. Fixed some coding style. Seems perfect to me.

andypost’s picture

Priority: Major » Critical
Status: Fixed » Needs work

Not sure that's fixed, all change notices that references to ControllerInterface needs to be updated

dawehner’s picture

Status: Needs work » Fixed
Wim Leers’s picture

yched’s picture

Since we're catering for DX here - If this class is only intended for controllers (and the docs in the file do seem to imply it is), this should show in the class name and/or namespace. It is IMO a WTF that Drupal\Core\DependencyInjection\ContainerInjectionInterface sounds like a totally generic thing that would be suitable for anything that does DI.

Also, the @file docblock in ContainerInjectionInterface.php still says
Contains \Drupal\Core\Controller\ContainerInjectionInterface,
which is wrong.

[edit: sorry, this last point has just been fixed separately - the naming issue still stands IMO]

Wim Leers’s picture

webchick’s picture

Also, #43 mentions #2076085: Resolve the need for a 'title callback' using the route would be a non-controller use of this class, justifying its more generic name.

yched’s picture

OK, but then the docs in the class are misleading ?

/**
 * Defines a common interface for route controllers.
 *
 * This interface gives controller classes a factory method for instantiation
 * rather than relying on a services.yml entry. However, it may result in
 * a lot of boilerplate code in the class. As an alternative, controllers that
 * contain only limited glue code ("thin" controllers) should instead extend
 * ControllerBase as that allows direct access to the container. That renders
 * the controller very difficult to unit test so should only be used for
 * controllers that are trivial in complexity.
 */
interface ContainerInjectionInterface {
webchick’s picture

Title: Rename ControllerInterface to ContainerInjectionInterface » [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface
Priority: Critical » Normal
Status: Fixed » Needs work

Well THAT is a fine question. :) I guess because #2076085: Resolve the need for a 'title callback' using the route is not in yet, but since it's critical, it'll go in at some point, so probably makes sense to clean those docs up here.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Fixed

We have an updated documentation already:

/**
 * Defines a common interface for dependency container injection.
 *
 * This interface gives classes who need services a factory method for
 * instantiation rather than defining a new service.
 */

Status: Fixed » Closed (fixed)

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