this is really weird. I never saw such behavior (and I saw errors for unknown ids though, which is even covered by tests)

class DefaultController extends Controller
{
    public function indexAction(Request $request)
    {
        $test= $this->get('test.with_underscores');
        var_dump($test);
        $test= $this->get('test.withunderscores');
        var_dump($test);
        $test= $this->get('test.with_underscores');
        var_dump($test);
        exit;
    }
}

outputs:

object(Bundle\TestBundle\Manager\TestManager) object id 272
(class info here)
object(Bundle\TestBundle\Manager\TestManager) object id 444
(class info here)
object(Bundle\TestBundle\Manager\TestManager) object id 444
(class info here)

Only works with underscores. If anything other changes it will give a error.

Properly caused by the strtr() in
Symfony\Component\DependencyInjection\Container.php:216
} elseif (method_exists($this, $method = 'get'.strtr($id, array('' => '', '.' => '', '' => '_')).'Service')) {

Where '_' are replaced by ''

No tests found for errorhandling on naming with a underscore
ContainerTest.php:205

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Container.php#L216 This is only done for synchronized services.

What is the exact service definition of your service?

Here also: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Container.php#L301

in services.php

$container->setDefinition('test.with_underscores', new Definition('Bundle\TestBundle\Manager\TestManager'))
    ->addArgument(new Reference('doctrine.orm.entity_manager'));

Think that PHP method name resolution is case insensitive.

For the method_exists() function:

"FooBar" == "Foobar"  // method name
"foo_bar" == "foobar" // service id

To fix it the method_exists call should be avoided in favour of a method map only.

Using a method map would not help allowing to have foobar and foo_bar in the container at the same time

@stof indeed, it's not possible to have foobar and foo_bar in the container at the same time defined by method.

Stop me if I'm wrong but to prevent collision we can do something.

On prerequisite the service id foo_bar is defined but not foobar:

• ::get() should throw ServiceNotFoundException when called with foobar code.
• ::has() should return False when called with foobar code.
• ::set() should not synchronize foo_bar when called with foobar code.

As this is an edge which would break BC if we try to fix it, I think it's safe to mark this one as a won't fix.

@fabpot Given that these generated methods are internal APIs, we could use a different replacement for underscores rather then using the empty char (the current generation turns snake case ids into camel cased method names, but this does not play well with the fact that PHP methods are case insensitive)

@stof Indeed, we could keep the underscore.

except we currently use the underscore to replace dots (which would then conflict). We need to find another way to replace dots in such case

What needs to be done here?

It’s a highly mind-boggling problem if the affected service has an event listener (or maybe also a compiler pass) affecting it: getting the service with a missing or extra underscore in the name will give you a new instance that hasn’t had the listener run on it. It can take a while to determine where the problem comes from.

So yes, it’s an edge case, but a very time-consuming one.

How performant does the conversion from service name to method name have to be? Could strtr be replaced by something else? The conversion is done every time get or has is called, but that’s not something that should be done inside a loop, for example, so maybe a small performance hit would be acceptable?

Well, we are now using a method map to handle. The remaining thing to fix this issue would be to make it the authoritative way for compiled container by dropping entirely the check for protected methods on the container.
@fabpot @nicolas-grekas what do you think about this change ? I'm all in favor of it. Regarding BC, it would break things only for people writing their own dumper for compiled container. And if we want to preserve BC, we could keep the existing logic, but only when the methodMap is empty (throwing a deprecation notice saying that the dumper should be updated to the new way)

Looks like a good idea. I don't think we'd need to keep BC at this level, extending the generated protected methods would be really fragile anyway imho.

the question is not about extending the generated protected methods, but about the way the list of compiled services are detected.
In previous versions (before the introduction of the methodMap to solve a related issue), the logic was based purely on detecting whether the factory method was defined in the class. Today, we could use the methodMap exclusively for that (and it may be faster by avoiding to do string transformations but doing a simple key lookup in an array). But if people are using the component standalone by writing a container by hand, they might not define the methodMap, and relying on the method detection logic.

Btw, I think keeping BC would not be hard. And the impact for people using the dumped container in Symfony would be limited to a if(empty($this->methodMap) extra check (returning false and so no extra logic) when trying to access (or check has()) for a non-existent service. Any known service would already return earlier anyway.

LGTM, let's try this way, BC bridge included then!

I will work on it

here you go

A big (belated) thank you for your work on this. I would have liked to tackle it, but I lacked some understanding of how the container works. And also, you were so quick. ;)