Updated: Comment #0

Problem/Motivation

FormBase implements ContainerInjectionInterface and provides an empty create() method as a default. ControllerBase, on the other hand, does not implement ContainerInjectionInterface.

This is confusing to have these otherwise-similar base classes diverging here. In addition, needing ContainerInjectionInterface is extremely common, and implementing it by default does not add any real overhead.

Proposed resolution

Add ContainerInjectionInterface to ControllerBase
Switch to ControllerBase instead of ContainerInjectionInterface where appropriate, for easing future improvements, and remove the explicit mention of ContainerInjectionInterface for classes that already extend ControllerBase or FormBase

Remaining tasks

N/A

API changes

When extending ControllerBase, you no longer need to explicitly implement ContainerInjectionInterface

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
35.26 KB

First try.

Status: Needs review » Needs work

The last submitted patch, 2110501-1.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
6.64 KB
41.9 KB

That was not two bad. Let's see.

dawehner’s picture

Status: Needs review » Needs work

Sadly this patch does not apply anymore otherwise I would have liked to set it to RTBC

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.65 KB
41.93 KB

There were just some changed use statements. Attached a diff of the two patches.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

All changes are perfectly in scope.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Are we sure about this - FormBase is a convenience class most things need more that what is provided - and you can just implement FormInterface directly.

tim.plunkett’s picture

This is a complete won't fix from me. Just look at that patch! The reverse of it would seem like a great refactoring.

tim.plunkett’s picture

Title: FormBase should not implement ContainerInjectionInterface » ControllerBase should implement ContainerInjectionInterface like FormBase
Issue tags: +DX (Developer Experience), +FormBase
FileSize
19.54 KB

If you finish writing your new form, extending FormBase, but you need to refactor and inject some services, how do you do it?
You look at FormBase and see (or your IDE tells you) a method called create() that gets the container. You override it, and are done.

With this patch, you don't have that luxury. You have to somehow find ContainerInjectionInterface, in the Drupal\Core\DependencyInjection namespace, and implement it, and its method.

The entire purpose of FormBase is to simplify the act of writing a form. It's completely optional, and none of this is required. FormInterface and ContainerInjectionInterface can be used separately. But a base class provided for convenience should be convenient.

The one valid point @dawehner raised was a lack of consistency. Why should this be easier in FormBase but harder in ControllerBase? That's a very good point, which led me to rename and repurpose this issue.

Instead of removing parts of FormBase, let's just make the same improvements to ControllerBase.

The diffstat for #5 was +97,-57.
This diffstat is +33,-45.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Issue summary: View changes
tim.plunkett’s picture

Actually because of #2110643: ControllerBase::container() and FormBase::container() needs to be private, this needs to be major as well. Almost every module dev will write a page callback.

tim.plunkett’s picture

Priority: Normal » Major

...

tim.plunkett’s picture

FileSize
50.57 KB

Here's the full patch. This borrows the fix to buildForm() contained in #2147669: Convert exposed forms to use FormInterface, I might split that off to its own issue.

alexpott’s picture

Tagging. We certainly need an issue summary update as the title and the body contradict each other!

To me it makes sense for ControllerBase to implement ContainerInjectionInterface.

tim.plunkett’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
tim.plunkett’s picture

FileSize
50.61 KB

Rerolled, conflict on use statements.

xjm’s picture

Priority: Major » Critical
Issue tags: +beta blocker

Discussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.

dawehner’s picture

Don't get me wrong I totally like the patch, but can someone explain me why this is critial? This saves people the following two lines of code, which is nothing compared to anything else. The ContainerInjectionInterface is also a concept noone will be able to skip, as it comes up everywhere.

use Drupal\Core\DependencyInjection\ContainerInjectionInterface;

implements ContainerInjectionInterface {
  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -196,6 +196,9 @@ public function buildForm($form_id, &$form_state) {
         }
    
    +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
    @@ -15,7 +15,7 @@
    diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
    
    diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
    index 99a2d73..01b1385 100644
    
    index 99a2d73..01b1385 100644
    --- a/core/lib/Drupal/Core/Form/FormBuilder.php
    
    --- a/core/lib/Drupal/Core/Form/FormBuilder.php
    +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    
    +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -196,6 +196,9 @@ public function buildForm($form_id, &$form_state) {
    
    @@ -196,6 +196,9 @@ public function buildForm($form_id, &$form_state) {
         // Ensure some defaults; if already set they will not be overridden.
         $form_state += $this->getFormStateDefaults();
     
    +    // Ensure the form ID is prepared.
    +    $form_id = $this->getFormId($form_id, $form_state);
    +
         if (!isset($form_state['input'])) {
           $form_state['input'] = $form_state['method'] == 'get' ? $_GET : $_POST;
    

    I am sorry but this feels kind of out of scope?

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
    @@ -9,19 +9,15 @@
    -use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
    ...
     use Drupal\Core\Controller\ControllerBase;
    -use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
     use Drupal\aggregator\FeedInterface;
    -use Drupal\aggregator\ItemInterface;
     use Drupal\Core\Database\Connection;
     use Symfony\Component\DependencyInjection\ContainerInterface;
    -use Symfony\Component\HttpFoundation\Request;
     use Symfony\Component\HttpFoundation\Response;
    ...
     
    

    If you want to make patches like this easy to review, just don't remove the use statemens, that are out of scope ... any opinions?

tim.plunkett’s picture

Status: Needs review » Postponed
Related issues: +#2147669: Convert exposed forms to use FormInterface

See #13, I had to borrow from another issue, so this is blocked on that.

And yes, I got a little overenthusiastic with my use statement removal, I'll fix that.

catch’s picture

Status: Postponed » Needs work

Looks like this is unblocked?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
51.14 KB

Yep, thanks for reminding me.
Rerolled and only removed use statements that are directly related to this patch.

dawehner’s picture

+++ b/core/modules/block/lib/Drupal/block/Controller/CategoryAutocompleteController.php
@@ -17,7 +17,7 @@
-class CategoryAutocompleteController implements ContainerInjectionInterface {
+class CategoryAutocompleteController extends ControllerBase {

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -19,7 +19,7 @@
-class BookController implements ContainerInjectionInterface {
+class BookController extends ControllerBase {

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -15,7 +15,7 @@
-class EntityViewController implements ContainerInjectionInterface {
+class EntityViewController extends ControllerBase {

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -17,7 +17,7 @@
-class ConfigController implements ContainerInjectionInterface {
+class ConfigController extends ControllerBase {

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceController.php
@@ -7,18 +7,17 @@
-class EntityReferenceController implements ContainerInjectionInterface {
+class EntityReferenceController extends ControllerBase {

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.php
@@ -21,7 +20,7 @@
-class ForumController implements ContainerInjectionInterface {
+class ForumController extends ControllerBase {

+++ b/core/modules/help/lib/Drupal/help/Controller/HelpController.php
@@ -15,7 +16,7 @@
-class HelpController implements ContainerInjectionInterface {
+class HelpController extends ControllerBase {

Just some examples: They don't use a SINGLE service from the ControllerBase, so this increases coupling of your code.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
pwolanin’s picture

created #2167169: Clean up Drupal\edit\EditController: remove ContainerAware and inject form_builder for improving \Drupal\edit\EditController - I don't think it needs to be part of this patch.

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Here's a revised patch in response to dawehner which either leaves controllers alone, or makes sure they are using something from ControllerBase

The last submitted patch, 26: controller-2110501-26.patch, failed testing.

pwolanin’s picture

FileSize
49.33 KB

re-roll for a change in HEAD (resolved by rebasing, no actual conflict)

Status: Needs review » Needs work

The last submitted patch, 28: controller-2110501-28.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

28: controller-2110501-28.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 28: controller-2110501-28.patch, failed testing.

The last submitted patch, 28: controller-2110501-28.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
47.47 KB

Having trouble reproducing it locally, but looking at the code I'm 99% sure the fatal is from core/tests/Drupal/Tests/Core/Entity/Controller/EntityViewControllerTest.php since that tries to inject the entity manager into the constructor, and so the container is missing.

So rolling back the change to the EntityViewController for now to see if the rest pass. No major need to convert that class anyhow.

dawehner’s picture

  1. +++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
    index e0f1d0f..ee224aa 100644
    --- a/core/modules/entity/lib/Drupal/entity/Controller/EntityDisplayModeController.php
    

    This class basically has the same issue.

  2. +++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.php
    @@ -23,7 +22,7 @@
    -class ImageStyleDownloadController extends FileDownloadController implements ContainerInjectionInterface {
    +class ImageStyleDownloadController extends FileDownloadController {
    
    @@ -8,7 +8,6 @@
    -use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    
    +++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectAddForm.php
    @@ -15,7 +15,7 @@
    -class ImageEffectAddForm extends ImageEffectFormBase implements ContainerInjectionInterface {
    +class ImageEffectAddForm extends ImageEffectFormBase {
    
    +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.php
    @@ -20,7 +19,7 @@
      */
    ...
    +class ContactController extends ControllerBase {
    ...
    -class ContactController extends ControllerBase implements ContainerInjectionInterface {
    
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    --- a/core/modules/contact/lib/Drupal/contact/Controller/ContactController.php
    +++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.php
    
    diff --git a/core/modules/contact/lib/Drupal/contact/Controller/ContactController.php b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.php
    index ff5f71d..213cae4 100644
    
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -9,14 +9,13 @@
    +class ConfigTranslationListController extends ControllerBase {
    ...
    -class ConfigTranslationListController extends ControllerBase implements ContainerInjectionInterface {
    ...
    -use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
    index 14bd763..42b2f6f 100644
    --- a/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    
    --- a/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    

    Aren't that another set of routes where we don't really profit from the Controller Base class?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I am just wrong.

catch’s picture

Title: ControllerBase should implement ContainerInjectionInterface like FormBase » Change notice: ControllerBase should implement ContainerInjectionInterface like FormBase
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

This is great. Committed/pushed to 8.x, thanks!

dawehner’s picture

pwolanin’s picture

Title: Change notice: ControllerBase should implement ContainerInjectionInterface like FormBase » ControllerBase should implement ContainerInjectionInterface like FormBase
Status: Active » Fixed
tstoeckler’s picture

Wait, so now we have two change notices?!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Status: Closed (fixed) » Reviewed & tested by the community
FileSize
61.72 KB

This was never committed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: controller-2110501-41.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
62.15 KB
705 bytes
larowlan’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed for real this time. :) Thanks!

Status: Fixed » Closed (fixed)

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