Problem/Motivation
$container->get('foo.bar')
is impossible to understand and IDE integrate. We need a container that allows for $container->fooBar()
and fooBar()
has a @return \the\name\of\the\class
. The current wrappers only help with IDE integration a little \Drupal::fooBar()->
[autocomplete] works but if you want to look inside the resulting \Drupal::foobar()->someMethod()
you can't -- at least not without first consulting Drupal::foobar() for the service ID and the service YAML containing the definition of foo.bar -- because you only have an interface not the class. And you want to look inside for understanding, debugging etc.
Proposed resolution
\Drupal::getContainer()->get('foo.bar')->baz()
becomes
\Drupal::getContainer()->fooBar()->baz()
Remaining tasks
The compiled container gets very close:
- Remove get from method names
- Make methods public.
- Make the classname something constant like
Drupal
and remove the current less useful Drupal class.
User interface changes
None.
API changes
Wheee! There are 854 occurences of container->get
in core/lib and core/modules and hopefully we can just script them. (The sheer amount of these calls, by the way, completely kills the argument that you won't need to deal with the container directly.)
There are also 2182 Drupal::
calls -- particularly interesting are the 274 Drupal::service and 30 getContainer calls.
Related Issues
#1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination needs to be done so that you actually can read the compiled class.
Comments
Comment #0.0
chx CreditAttribution: chx commentedadded related
Comment #1
dawehnerThe problem of having no autocompletion just appears in old procedural code and in factory methods. Everywhere else you do have at least interfaces available.
Having access to the container thought could also scare some people.
Note: Most of them are in create methodss, so they don't really count.
Comment #2
chx CreditAttribution: chx commentedAccording to
git grep Drupal::|wc -l
there are 2299 occurences of Drupal:: in HEAD as well and all of them would be improved. Also, just because those calls are in create it doesn't mean it's not helpful to make it more visible what's available, what class they are using and so on. Just the interface, while useful and adequate for anyone not wanting to peek under the hood is not enough.Comment #2.0
chx CreditAttribution: chx commentedadded a bit
Comment #2.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #2.2
chx CreditAttribution: chx commentedupdated Drupal::
Comment #2.3
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #2.4
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #2.5
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #3
chx CreditAttribution: chx commentedI am doing migrate so if someone else wants to work on this, that'd be great. I am glad to help of course
Comment #4
nicxvan CreditAttribution: nicxvan commentedThis may be a bit ambitious, but I am going to attempt this in the next couple of days and see where I end up.
Comment #5
Cameron Tod CreditAttribution: Cameron Tod commentedJust to let you know nicxvan, I have been working on this too, and have been communicating with chx in #drupal-contribute. Perhaps we could work in a sandbox on it together?
Comment #6
Cameron Tod CreditAttribution: Cameron Tod commentedJust to let you know nicxvan, I have been working on this too, and have been communicating with chx in #drupal-contribute. Perhaps we could work in a sandbox on it together?
Comment #7
nicxvan CreditAttribution: nicxvan commentedcam8001 we can collaborate on this. I'll be in drupal-contribute most of tomorrow if you want to grab me and let me know what you've gotten so far.
Comment #8
Crell CreditAttribution: Crell commentedEr. Why are we going to so much trouble to make the service container a friendlier service-locator, when we should be using it as a pure DI most of the time anyway? Most services you'd want to access from procedural code legitimately already have Drupal::whatever() methods anyway, which are nicely documented with an appropriate return type.
Comment #9
tstoecklerRe #8: Because the way we currently do DI is that you have to call $container->get('foo') a lot in a static create() function. It would be nice to have autocompletion, typed return values, etc. for that.
Comment #10
Cameron Tod CreditAttribution: Cameron Tod commentedAdding tag for the London sprint! https://drupalsprintoctober2013.eventbrite.co.uk/
Comment #11
Crell CreditAttribution: Crell commented#9: But in 98% of cases all you're doing there is returning the service straight into a function call. There's no autocomplete to be had there, since you're not calling methods on the service.
This moves us too far to encouraging a service-locator rather than proper DI.
Comment #12
chx CreditAttribution: chx commented> There are also 2182 Drupal:: calls -- particularly interesting are the 274 Drupal::service and 30 getContainer calls.
Eh. Drupal 8 will still ship with hooks. You can't avoid hooks. But you need the services. What now? Are you going to write a separate proper DI'd class (either by definign it as a service or using some factory) for every. single. hook implementation?
Comment #13
Cameron Tod CreditAttribution: Cameron Tod commentedAfter trying with custom proxy dumpers, and attempting to do a lot of the conversion work, I've decided to just upload just a basic implementation. This gives full service discoverability in PHPStorm.
Comment #14
Cameron Tod CreditAttribution: Cameron Tod commentedWoops, forgot to add in the new dumper.
Comment #16
Cameron Tod CreditAttribution: Cameron Tod commentedThese two should go green.
In one of the patches, the "get" prefix is removed from accessor methods in the service container. This allows you to do \Drupal::getContainer()->LanguageManagerService instead of \Drupal::getContainer()->getLanguageManagerService(). That approach required hacking away at the Symfony container in a way that felt a bit clunky, so I've posted a simpler patch that maintains the get prefix.
Comment #17
BerdirWe can't use those methods as they are right now. Those create a *new* instance of a service and circumvent the static cache of get() and the other logic in there, like checking if a a service is there (that you had that method while developing doesn't mean it will be there on a different installation) and recursion detection. And I don't think we want to add all that to every single method?
Comment #18
Cameron Tod CreditAttribution: Cameron Tod commentedRight you are, there's a lot of useful stuff going on in Container->get() which is not nice to lose. Do you think there is a way that we can have code completion and maintain this?
Comment #18.0
Cameron Tod CreditAttribution: Cameron Tod commentedbefore-after code sample
Comment #19
dawehnerDid someone already filled an issue against symfony upstream in order to be able to replace some of these methods?
What about keeping get{$service}Service methods and let our {$service}Service just wrap get()?
Comment #20
sun+1 for doing this.
IIRC, I actually tried to do this in the past already (but not sure whether it ever emerged into an d.o issue).
The logic in
get()
definitely has to be retained and invoked though.In essence, we want to generate new methods on the dumped Container like this:
As a side benefit, doing this will also reveal that a whole bunch of service names are not using the right combination of underscores (_) and dots (.) - for example:
Comment #21
catchI don't think the usage of the container in ::create() is enough to make this critical, and that's the only place where the container is really, really non-optional, so downgrading.
Comment #22
xjm16: 2084637-service-container-no-get-prefix-15.patch queued for re-testing.
Comment #23
xjm16: 2084637-service-container-with-get-prefix-15.patch queued for re-testing.
Comment #26
dawehnerSo we now have IDE integration, do we need more?
Comment #27
jibranSpeaking of IDE, Devel has a drush command phpm(phpstorm-metadata) it saves the PhpStorm metadata file to Drupal root. See http://confluence.jetbrains.com/display/PhpStorm/PhpStorm+Advanced+Metadata for more details. Also, I have created an issue in devel iq to improve the output of command #2419235: phpstorm-metadata should only dump all services objects..
Yeah, it is won't fix now as per #26 IMHO.
Comment #28
dawehnerYeah, we have pretty good support now. There is also a dedicated PhpStorm extension which does a hell lot of things.