Use a service container with automated wrappers

The 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.

(The sheer amount of these calls, by the way, completely kills the argument that you won't need to deal with the container directly.)

Note: Most of them are in create methodss, so they don't really count.

This may be a bit ambitious, but I am going to attempt this in the next couple of days and see where I end up.

Just 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?

Just 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?

cam8001 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.

Er. 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.

Re #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.

Adding tag for the London sprint! https://drupalsprintoctober2013.eventbrite.co.uk/

#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.

After 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.

Woops, forgot to add in the new dumper.

These 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.

We 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?

Right 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?

before-after code sample

Did 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()?

+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:

/**
 * Gets the 'entity.manager' service.
 *
 * @return Drupal\Core\Entity\EntityManager
 */
public function getEntityManager() {
  return $this->get('entity_manager');
}

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:

Current name/method                          →  Proper name/method

entity.manager                               →  entity_manager
  getEntity_ManagerService()                 →    getEntityManagerService()

plugin.manager.field.formatter               →  plugin_manager.field_formatter
  getPlugin_Manager_Field_FormatterService() →    getPluginManager_FieldFormatterService()

I 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.

16: 2084637-service-container-no-get-prefix-15.patch queued for re-testing.

16: 2084637-service-container-with-get-prefix-15.patch queued for re-testing.

So we now have IDE integration, do we need more?

Speaking 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.

Yeah, we have pretty good support now. There is also a dedicated PhpStorm extension which does a hell lot of things.