We went with a different technique on FormBase (#2059245: Add a FormBase class containing useful methods) which doesn't require the class to be container aware.

Here's a first pass at making them more consistent.

Files: 
CommentFileSizeAuthor
#26 controller-2077513-26.patch19.78 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es).
[ View ]
#26 interdiff.txt798 bytesParisLiakos
#24 controller-2077513-24.patch19 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,594 pass(es), 174 fail(s), and 318 exception(s).
[ View ]
#24 interdiff.txt1.93 KBParisLiakos
#20 controller-2077513-20.patch18.68 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,023 pass(es).
[ View ]
#20 interdiff.txt11.59 KBtim.plunkett
#16 controller-2077513-16.patch8.3 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,433 pass(es), 10 fail(s), and 2 exception(s).
[ View ]
#14 drupal8.base-system.2077513-14.patch8.15 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,222 pass(es), 102 fail(s), and 71 exception(s).
[ View ]
#10 drupal8.base-system.2077513-10.patch8.16 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2077513-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 controller-2077513-5.patch8.1 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,489 pass(es).
[ View ]
#5 interdiff.txt1.35 KBtim.plunkett
#2 controllerbase_refactor-2077513-2.patch7.3 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 57,270 pass(es), 591 fail(s), and 605 exception(s).
[ View ]
#2 interdiff.txt2.58 KBmsonnabaum
controllerbase_refactor.patch5.26 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 57,171 pass(es), 572 fail(s), and 1,012 exception(s).
[ View ]

Comments

One of the main reasons FormBase doesn't extend ControllerBase was because I didn't want FormBase to be ContainerAware.
Do we want to change that now?

  1. +++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
    @@ -38,7 +37,10 @@
    +    if (!$this->entityManager) {
    @@ -72,7 +74,10 @@ protected function cache($bin = 'cache') {
    +    if (!$this->configFactory) {
    @@ -99,7 +107,10 @@ protected function keyValue($collection) {
    +    if (!$this->stateService) {
    @@ -108,7 +119,10 @@ protected function state() {
    +    if (!$this->moduleHandler) {
    @@ -118,7 +132,10 @@ protected function moduleHandler() {
    +    if (!$this->urlGenerator) {
    @@ -128,7 +145,10 @@ protected function urlGenerator() {
    +    if (!$this->currentUser) {
    @@ -151,7 +171,20 @@ protected function currentUser() {
    +    if (!$this->translationManager) {
    @@ -161,7 +194,23 @@ protected function t($string, array $args = array(), array $options = array()) {
    +    if (!$this->languageManager) {

    We should add protected properties for these.

  2. +++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
    @@ -161,7 +194,23 @@ protected function t($string, array $args = array(), array $options = array()) {
    +  protected function container() {

    I like this better than the \Drupal::service calls in FormBase, can we swap those over too?

  3. +++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
    @@ -161,7 +194,23 @@ protected function t($string, array $args = array(), array $options = array()) {
    +    if (!$this->container) {
    +      $this->container = \Drupal::getContainer();
    +    }
    +    return $this->container;

    Let's not store $this->container at all

StatusFileSize
new2.58 KB
new7.3 KB
FAILED: [[SimpleTest]]: [MySQL] 57,270 pass(es), 591 fail(s), and 605 exception(s).
[ View ]

Agreed.

Issue tags:+WSCCI, +FormInterface, +FormBase

Well, I'm happy.

I think some subclasses might be relying on $this->container, so this might fail.

Status:Needs review» Needs work

The last submitted patch, controllerbase_refactor-2077513-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.35 KB
new8.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,489 pass(es).
[ View ]

Let's see if we have some more real failures.

What exactly is the point here? Note that we lose access to other services in the controller with this approach. That means we then are looking at potentially classes having both ControllerBase and ControllerInterface-cum-whatever-it-is. That's starting to get ridiculous. :-)

ControllerBase is for "thin, not worth testing" controllers. ContainerAware is acceptable there.

FormBase takes a different tactic because forms are not "thin, not worth testing" code. We do want to bother testing those properly.

Note that we lose access to other services in the controller with this approach

What? How? I don't see that at all. Write your own wrapper for a service and call $this->container() just like these.

That means we then are looking at potentially classes having both ControllerBase and ControllerInterface

class DbLogController extends ControllerBase implements ControllerInterface
class SystemController extends ControllerBase implements ControllerInterface

Too late.

I have seen some really strange code in recent conversions that either expect FormBase to be ContainerAware, or ControllerBase issues just thinking "yay, I can use \Drupal:: now!"

This doesn't change the testability of anything.

Status:Needs review» Reviewed & tested by the community

Too late.

Sigh.

OK, looking over the patch itself rather than the discussion, this makes more sense to me. I'm still concerned about ending up with thick controllers that access the container directly but... I guess that's going to be a training question either way.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs reroll

git ac https://drupal.org/files/controller-2077513-5.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8296  100  8296    0     0   8055      0  0:00:01  0:00:01 --:--:--  9817
error: patch failed: core/lib/Drupal/Core/Controller/ControllerBase.php:29
error: core/lib/Drupal/Core/Controller/ControllerBase.php: patch does not apply

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new8.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2077513-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

3-way merge :).

PS: I am going to reroll some patches while @tim.plunkett is sleeping :D

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI, -FormInterface, -FormBase

The last submitted patch, drupal8.base-system.2077513-10.patch, failed testing.

Status:Needs work» Needs review

#10: drupal8.base-system.2077513-10.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+WSCCI, +FormInterface, +FormBase

The last submitted patch, drupal8.base-system.2077513-10.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new8.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,222 pass(es), 102 fail(s), and 71 exception(s).
[ View ]

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.base-system.2077513-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.3 KB
FAILED: [[SimpleTest]]: [MySQL] 58,433 pass(es), 10 fail(s), and 2 exception(s).
[ View ]

Reroll.

seems like it would be better to use \Drupal\Core\Controller\ContainerInjectionInterface instead of calling \Drupal::container()

Nope. That breaks the entire purpose of this.
Then any subclass would have to reimplement create() and pass through the same things as the parent, which is the entire reason we introduced this stuff.

Status:Needs review» Needs work

The last submitted patch, controller-2077513-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.59 KB
new18.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,023 pass(es).
[ View ]

The discrepancies in naming have already come up in some issues (getCurrentUser vs currentUser), so I'm unifying them here.∫

Status:Needs review» Reviewed & tested by the community

Back to rtbc.

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -29,7 +28,52 @@
+  /**
+   * @var \Drupal\Core\Entity\EntityManager
+   */
+  protected $entityManager;
+
+  /**
+   * @var \Drupal\Core\Language\LanguageManager
+   */
+  protected $languageManager;
+
+  /**
+   * @var \Drupal\Core\StringTranslation\TranslationInterface
+   */
+  protected $translationManager;
+
+  /**
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $configFactory;
+
+  /**
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
+   */
+  protected $kv;
+
+  /**
+   * @var \Drupal\Core\Routing\UrlGeneratorInterface
+   */
+  protected $urlGenerator;
+
+  /**
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $currentUser;
+
+  /**
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
+   */
+  protected $stateService;
+
+  /**
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;

Is there a reason why we don't have single lines at the top? $kv also seems a bad variable name.

Status:Reviewed & tested by the community» Needs work

So I'm guessing the @dawehner wanted to set this back to needs work

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
new19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,594 pass(es), 174 fail(s), and 318 exception(s).
[ View ]

lets get this in asap.
it confuses the shit out of me in its current form on what the best practices are when extending it and i cant properly review conversion issues.

Status:Needs review» Needs work

The last submitted patch, controller-2077513-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new798 bytes
new19.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Thanks for the rerolls!

Back to RTBC as #22 is addressed.

Status:Reviewed & tested by the community» Fixed

Awesome! Committed and pushed to 8.x. Thanks!

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