Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

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

msonnabaum’s picture

tim.plunkett’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
8.1 KB

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

Crell’s picture

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.

tim.plunkett’s picture

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.

Crell’s picture

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.

alexpott’s picture

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
jibran’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
8.16 KB

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.

star-szr’s picture

Status: Needs work » Needs review

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

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

jibran’s picture

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

Status: Reviewed & tested by the community » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.3 KB

Reroll.

pwolanin’s picture

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.59 KB
18.68 KB

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc.

dawehner’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
19 KB

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.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
798 bytes
19.78 KB
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the rerolls!

jibran’s picture

Back to RTBC as #22 is addressed.

webchick’s picture

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.