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
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 705 bytes | tim.plunkett |
#43 | controller-2110501-43.patch | 62.15 KB | tim.plunkett |
Comments
Comment #1
tstoecklerFirst try.
Comment #3
tstoecklerThat was not two bad. Let's see.
Comment #4
dawehnerSadly this patch does not apply anymore otherwise I would have liked to set it to RTBC
Comment #5
tstoecklerThere were just some changed use statements. Attached a diff of the two patches.
Comment #6
dawehnerAll changes are perfectly in scope.
Comment #7
alexpottAre we sure about this - FormBase is a convenience class most things need more that what is provided - and you can just implement FormInterface directly.
Comment #8
tim.plunkettThis is a complete won't fix from me. Just look at that patch! The reverse of it would seem like a great refactoring.
Comment #9
tim.plunkettIf 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.
Comment #10
tim.plunkettComment #11
tim.plunkettActually 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.
Comment #12
tim.plunkett...
Comment #13
tim.plunkettHere'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.
Comment #14
alexpottTagging. 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.
Comment #15
tim.plunkettComment #16
tim.plunkettRerolled, conflict on use statements.
Comment #17
xjmDiscussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.
Comment #18
dawehnerDon'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.
I am sorry but this feels kind of out of scope?
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?
Comment #19
tim.plunkettSee #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.
Comment #20
catchLooks like this is unblocked?
Comment #21
tim.plunkettYep, thanks for reminding me.
Rerolled and only removed use statements that are directly related to this patch.
Comment #22
dawehnerJust some examples: They don't use a SINGLE service from the ControllerBase, so this increases coupling of your code.
Comment #23
tim.plunkettComment #24
pwolanin CreditAttribution: pwolanin commentedcreated #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.
Comment #25
pwolanin CreditAttribution: pwolanin commentedComment #26
pwolanin CreditAttribution: pwolanin commentedHere's a revised patch in response to dawehner which either leaves controllers alone, or makes sure they are using something from ControllerBase
Comment #28
pwolanin CreditAttribution: pwolanin commentedre-roll for a change in HEAD (resolved by rebasing, no actual conflict)
Comment #30
pwolanin CreditAttribution: pwolanin commented28: controller-2110501-28.patch queued for re-testing.
Comment #33
pwolanin CreditAttribution: pwolanin commentedHaving 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.
Comment #34
dawehnerThis class basically has the same issue.
Aren't that another set of routes where we don't really profit from the Controller Base class?
Comment #35
dawehnerI am just wrong.
Comment #36
catchThis is great. Committed/pushed to 8.x, thanks!
Comment #37
dawehnerhttps://drupal.org/node/2168121
Comment #38
pwolanin CreditAttribution: pwolanin commentedchange notice: https://drupal.org/node/2168147
Comment #39
tstoecklerWait, so now we have two change notices?!
Comment #41
tim.plunkettThis was never committed.
Comment #43
tim.plunkettComment #44
larowlanComment #45
webchickCommitted and pushed for real this time. :) Thanks!