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.
We went with a different technique on FormBase (#2059245: Add a FormBase class containing useful methods) which doesn't require the class to be container aware.
Here's a first pass at making them more consistent.
Comment | File | Size | Author |
---|---|---|---|
#26 | controller-2077513-26.patch | 19.78 KB | ParisLiakos |
#26 | interdiff.txt | 798 bytes | ParisLiakos |
#24 | controller-2077513-24.patch | 19 KB | ParisLiakos |
#24 | interdiff.txt | 1.93 KB | ParisLiakos |
#20 | controller-2077513-20.patch | 18.68 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettOne 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?
We should add protected properties for these.
I like this better than the \Drupal::service calls in FormBase, can we swap those over too?
Let's not store $this->container at all
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedAgreed.
Comment #3
tim.plunkettWell, I'm happy.
I think some subclasses might be relying on $this->container, so this might fail.
Comment #5
tim.plunkettLet's see if we have some more real failures.
Comment #6
Crell CreditAttribution: Crell commentedWhat 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.
Comment #7
tim.plunkettWhat? How? I don't see that at all. Write your own wrapper for a service and call $this->container() just like these.
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.
Comment #8
Crell CreditAttribution: Crell commentedSigh.
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.
Comment #9
alexpottNeeds reroll
Comment #10
jibran3-way merge :).
PS: I am going to reroll some patches while @tim.plunkett is sleeping :D
Comment #12
star-szr#10: drupal8.base-system.2077513-10.patch queued for re-testing.
Comment #14
jibranreroll after #2057869: Provide an alias for 'plugin.manager.entity' called 'entity.manager'.
Comment #16
tim.plunkettReroll.
Comment #17
pwolanin CreditAttribution: pwolanin commentedseems like it would be better to use \Drupal\Core\Controller\ContainerInjectionInterface instead of calling \Drupal::container()
Comment #18
tim.plunkettNope. 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.
Comment #20
tim.plunkettThe discrepancies in naming have already come up in some issues (getCurrentUser vs currentUser), so I'm unifying them here.∫
Comment #21
jibranBack to rtbc.
Comment #22
dawehnerIs there a reason why we don't have single lines at the top? $kv also seems a bad variable name.
Comment #23
alexpottSo I'm guessing the @dawehner wanted to set this back to needs work
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedlets 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.
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedComment #27
tim.plunkettThanks for the rerolls!
Comment #28
jibranBack to RTBC as #22 is addressed.
Comment #29
webchickAwesome! Committed and pushed to 8.x. Thanks!