This is a simple patch to allow for ContainerAware controllers.
That is, a controller (nee page callback) may at times be an object that implements the ContainerAwareInterface. That is, it wants to have the dependency injection container passed into it wholesale. Normally you don't want to do that, but controllers are supposed to be small glue-code things so it's easier than setting up the exact dependencies for each separate object.
With this patch, any controller that has that interface will automagically have the container injected into it when it's created. This is the same logic that Symfony fullstack uses.
This issue is forked off of #1606794: Implement new routing system, because it's small and self-contained enough to stand on its own.
Patch in a moment as soon as I have a nid. :-)
Comment | File | Size | Author |
---|---|---|---|
#23 | container-aware-1777430-22-FAIL.patch | 2.16 KB | tim.plunkett |
#23 | 1777430-container-aware-controller_3.patch | 2.61 KB | tim.plunkett |
#21 | 1777430-container-aware-controller.patch | 2.61 KB | Crell |
#18 | method_page_callback-FAIL.patch | 2.22 KB | tim.plunkett |
#18 | method_page_callback-PASS.patch | 2.66 KB | tim.plunkett |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch.
Comment #2
sunCan we copy the issue summary into the class' phpDoc, please? :)
We're generally missing explanations for the new abstract concepts and builders, and more importantly reasonings for why they exist and why they're useful, at which point they're used/invoked, and how they relate to other classes, and so on. That would be a huge help for people who're looking at some of the code/files to understand what's going on.
Symfony itself improved a lot in its latest version with regard to that, but overall, doesn't set a good example. (Other frameworks are providing much more elaborate documentation in code to explain the usage and concepts.)
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedOnce sun is happy with the docs, this looks RTBC to me.
Comment #5
Crell CreditAttribution: Crell commentedWell I don't want to do a direct c&p since I assume we'll want to do other things in the subclass later besides just this. But yeah, I can try to beef up the inline comments a bit. Would that be sufficient? (Down where ContainerAwareInterface is actually checked.)
I think sometime after code freeze I'm going to finally make good on my thread to organize a bunch of Drupalers and let them loose on the Symfony code base's docblocks. :-) It really does need some Drupal treatment, I agree.
Comment #6
sunoink - let's not overcomplicate it :) I was basically just asking to get from zero to something ;)
Let me try (not sure whether this is correct):
---
Comment #7
Crell CreditAttribution: Crell commentedRevised version with additional docs. I didn't do a copy paste because a class docblock is not the place to wax about good controller practices, but it does say what's going on and why. I think this file is now more comment than code. Poor fabpot would probably cry. :-)
Comment #8
fgmIn that version, there is no longer a warning about this not being a recommended practice, as in the original text. Which means passing the container wholesale is likely to become quite frequent when newcomers discover the class without a best practices warning, since it is so convenient (oooh, I can have the whole global configuration if I use this controller, let's do this !)
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedSmall nit... I think Drupal coding style calls for NULL (not null).
Comment #10
Crell CreditAttribution: Crell commentedBah, inherited from Symfony when I copy/pasted the code. Fixed.
fgm: I don't believe a class docblock is the right place to get into the nuance of when a controller should be ContainerAware and when a controller should itself be a service. That's a complicated issue; Symfony full stack supports both. (Service controllers would be a later patch to this class, probably.) Several leading Symfonians disagree on it. (Fabpot strongly favors ContainerAware controllers; lsmith strongly favors service controllers.)
Basically, a docblock on a class virtually no module developer will ever see is not the place to try and capture and codify that nuance. :-)
Comment #11
sunThanks!
I started a code review, but then I realized that I could just as well fix the issues myself. I usually prefer to just review, so the problems don't get repeated in other patches... hopefully the interdiff will suffice.
For the @file directive, the summary should appear on the next line. Should also end in a period.
"Constructs"
...
Comment #13
Crell CreditAttribution: Crell commentedI don't know what you did, but here it is again with the changes from #11. :-)
Comment #14
sunThe only changes are docs changes, as visible in the interdiff in #11.
I suspect we have another random test failure in Drupal\locale\Tests\LocaleFileImportStatus...
Comment #15
catchOK this looks reasonable to me, and thanks for the docs improvements. Committed/pushed to 8.x.
Comment #16
tim.plunkettThis broke the ability to use a method as a page callback: #1781560: Regression: Methods can no longer be used in page callbacks
Comment #17
Crell CreditAttribution: Crell commentedSigh. All that back and forth about code comments, and NONE of us realized that we forgot a return statement. :-(
Tim indicated in IRC that we should still have tests after fixing it. If so, it should be a unit test on Drupal\Core\ContainerResolver, not a functional test. In practice, nothing should be breaking here that was supposed to work yet, until after the routing patch gets in.
Comment #18
tim.plunkettHere is the functional test and test+fix. They can be replaced with unit tests, but should not be removed.
This should set the issue back to "needs work".
Comment #20
fgm(sorry: answered another thread and ended on that one, please ignore)
Comment #21
Crell CreditAttribution: Crell commentedSigh. With the correct test.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedthis looks like the patch i RTBC'ed over at #1781560: Regression: Methods can no longer be used in page callbacks.
Comment #23
tim.plunkettReuploading #21 with and without the tests, just to be sure the test works. But, RTBC because it makes sense.
Comment #24
webchickCommitted and pushed to 8.x. Thanks!