Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#52 | container-injection-2057589-52.patch | 46.71 KB | tim.plunkett |
#48 | interdiff.txt | 1.02 KB | tim.plunkett |
#48 | controllerinterface-2057589-48.patch | 47.6 KB | tim.plunkett |
#46 | controllerinterface-2057589-46.patch | 49.31 KB | tim.plunkett |
#46 | interdiff.txt | 12.11 KB | tim.plunkett |
Comments
Comment #1
alexpottThe -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
Comment #2
Crell CreditAttribution: Crell commenteds/ControllerInjectionInterface/CreateableControllerInterface/ :-)
Comment #3
xjmComment #4
xjmComment #5
webchickHere'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?
Comment #6
webchickAlso, FWIW, "CreateableControllerInterface" means absolutely nothing to me, and nothing in here:
...nor here:
...explains it, either.
Comment #7
dawehnerYeah creatable controller interface is a horrible name.
What about ContainerControllerInterface, as this is kind of similar to "ContainerFactoryPluginInterface"
Comment #8
alexpottOk 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 :)
Comment #9
alexpottUpdating title to current suggestion
Comment #10
Crell CreditAttribution: Crell commentedEh, 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).
Comment #11
webchickAgreed. 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.
Comment #12
catchTrivial 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.
Comment #13
dawehnerOn plugins it is named ContainerFactoryPluginInterface, so we could also go with that: ContainerFactoryControllerInterface
Comment #14
eojthebraveI 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 :)
Comment #15
msonnabaum CreditAttribution: msonnabaum commentedI'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.
Comment #16
smiletrl CreditAttribution: smiletrl commentedWord "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":)
Comment #17
dawehnerLet'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.
Comment #19
smiletrl CreditAttribution: smiletrl commentedThere'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.
Comment #20
smiletrl CreditAttribution: smiletrl commentedLooks like the first patch in #17 has been ignored by bot. Add it here.
Comment #22
xjm@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.
Comment #23
smiletrl CreditAttribution: smiletrl commented@xjm, yeah, you're right:)
Ok, rerolled the patch at #17 for "ContainerFactoryControllerInterface".
Comment #24
webchickOh, 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.
Comment #26
smiletrl CreditAttribution: smiletrl commentedfixed a bug.
Comment #28
dawehnerI think we tests we should not use the create method, but explicit pass the dependencies.
Comment #29
smiletrl CreditAttribution: smiletrl commentedHmm, not sure about this, is there some preference here, or to make the test more readable -- self-Documented?
Fixed more bugs.
Comment #31
smiletrl CreditAttribution: smiletrl commentedRerolled again. Drupal HEAD is going fast:) interdiff is the same with #29.
Comment #33
dawehnerEdit title.
Comment #34
dawehner> 80 chars.
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
Comment #35
msonnabaum CreditAttribution: msonnabaum commentedI've still not seen a justification for having the world controller in an interface that has nothing to do with controllers.
Comment #36
smiletrl CreditAttribution: smiletrl commentedok, 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.
Comment #37
Crell CreditAttribution: Crell commentedI'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.)
Comment #38
smiletrl CreditAttribution: smiletrl commentedContinuous work on ContainerFactoryControllerInterface. Name discussion is still open:)
Comment #40
smiletrl CreditAttribution: smiletrl commentedFix in this patch has passed failed tests in last patch locally. Let's see the online bot.
Comment #41
tim.plunkett@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.
Comment #42
smiletrl CreditAttribution: smiletrl commentedAny ideas about 'ContainerInjectionInterface', and put this interface inside name space -- Drupal\Core\DependencyInjection?
Comment #43
Crell CreditAttribution: Crell commentedIf #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.
Comment #44
tim.plunkett+1 to #42.
Rerolled.
Comment #45
dawehner/me points on tim for trusting phpstorm, HAHA
Comment #46
tim.plunkettLOL.
Comment #47
msonnabaum CreditAttribution: msonnabaum commentedI think this name is as good as we're going to get.
Comment #48
tim.plunkettRerolled.
Comment #49
catchI'm happy with this, don't want to commit after only a
couple of hoursfour minutes RTBC, so just posting an unassigning - I'll commit next time I'm looking through queue if no-one beats me to it.Comment #50
Crell CreditAttribution: Crell commentedI can work with #48. No further objections.
Comment #51
webchickAwesome!! Now we just need a re-roll.
Comment #52
tim.plunkettStraight reroll.
Comment #53
catchCommitted/pushed to 8.x, thanks!
Comment #54
yched CreditAttribution: yched commentedDrupal\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" ?
Comment #55
smiletrl CreditAttribution: smiletrl commentedYes, 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?
Comment #56
tim.plunkettThey have other required parameters, so they need a different interface.
Comment #57
Crell CreditAttribution: Crell commentedI have updated the WSCCI Conversion Guide here: https://drupal.org/node/1953342
Comment #58
smiletrl CreditAttribution: smiletrl commentedAdd new record here Rename ControllerInterface to ContainerInjectionInterface.
Comment #59
jibranThanks for the change notice. Fixed some coding style. Seems perfect to me.
Comment #60
andypostNot sure that's fixed, all change notices that references to
ControllerInterface
needs to be updatedComment #61
dawehnerI can't find other ones than https://drupal.org/node/2060189 and https://drupal.org/node/2023537
Comment #62
Wim LeersFollow-up bugfix: #2089851: ContainerInjectionInterface comment is wrong, causing developers to use the wrong "use" statement.
Comment #63
yched CreditAttribution: yched commentedSince 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]
Comment #64
Wim Leers#63: that's why I created #2089851: ContainerInjectionInterface comment is wrong, causing developers to use the wrong "use" statement: to fix that wrong docblock :)
Comment #65
webchickAlso, #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.
Comment #66
yched CreditAttribution: yched commentedOK, but then the docs in the class are misleading ?
Comment #67
webchickWell 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.
Comment #68
dawehnerWe have an updated documentation already: