$this->methodMap is a compilation artifact, thus any services there is/must be in $this->services.

Too quick comment, I didn't mean $this->services but all the get*Service() methods.

@nicolas-grekas technically, this change is needed, because get*Service methods may not allow to retrieve the original service id anymore in all case (as we change the generated name to exclude invalid chars and avoid conflicts). Symfony itself went creative regarding the method map, and forgot to check that getServiceIds keeps working with that.

On a side note, older versions were also buggy because the conversion between id to method name is ambiguous (multiple ids could generate the same get*Service method, which is also why we made the change in the generation).

so 👍 for this bugfix

@ro0NL please add a test ensuring that getServiceIds works fine when the method map used a different name to avoid conflicts (see PhpDumperTest::testConflictingServiceIds for such a setup generating a different name)

Technically, get*Service methods are also a compilation artifact. So maybe we should use the method instead to check for services.
Btw, this would be safer. Due to ambiguity in the generation of factory methods (multiple ids can generate the same method name), has could tell you that a service exist while the factory would actually be registering it under a different id, which will do weird things (the service will not be shared properly, and each call to get it will replace the shared instance of the real id)

@stof are you proposing to revert the last commit, and be safe in all cases?

please add a test ensuring that getServiceIds works fine when the method map used a different name to avoid conflicts

If i understand correctly, i kinda did that in edd63a7#diff-8aeba2b88d0347062017ee160a6beb49R619

However i considered it from a user perspective.. (ie. map how you want)

If i understand correctly, I kinda did that in edd63a7#diff-8aeba2b88d0347062017ee160a6beb49R619

yeah indeed. I missed it sorry.

I think we should be safe in all cases. Yes, it's a compilation artifact but it's more or less designed as a feature. The reverse lookup is needed for sure.

Leaving; lowercasing and merging remaining service ids. Both are related to "different method convention", so if we go safe now this wont bother us ever again :)

So, we have 2 issues then;

• state before compilation (we must depend on method_exists, get_class_method)
• state after compilation (we can depend on methodMap)

Where state depends on isCompiled/isFrozen. However, we dont actually compile the method map (ie. in Container::compile, which is bit weird maybe). Now suddenly state depends on the PhpDumper.

Basically we have no clue methodMap is available by design :) there is no isDumped method or so. Hence, we cover both cases?

I tend to agree with @nicolas-grekas we should only consider state after compilation (although we can do both, like currently). However, i think we must keep it consistent, ie; removing get_class_methods in getServiceIds, means removing method_exists in has.

So perhaps compiling methodMap should be moved to Container::compile first, to make things clear. Or just lazy load it :)

Here's a wild idea, make methodMap truly a compilation artifact by moving it all to the dumped container.

Cleanup anything "method" related in Container (ie method_exists, etc.), and override the affected methods in the dumped container, think;

// ProjectServiceContainer extends Container
protected $methodMap = array(
   'id' => 'getFooService' // just dumped as is
);

public function has($id)
{
    return isset($this->methodMap[$id]) || parent::has($id);
}

This seems to follow current design of the dumper... ie. i think this is the way to go (methodMap is no more a hidden feature of the Container).

@ro0NL your proposal does not work for get (it must go in the middle of the code), and can only work for has if we kill the optimization avoiding unnecessary strtolower calls.

True, get needs a little refactoring, ie. it should use a doInitialize, doHas, etc. that we can override.

Im not sure this approach is worth the effort.. but i do think it's better design opposed to Container being the almighty thing.

@ro0NL The little refactoring needed would involve dropping the performance optimization added a few months ago. This is not a good idea IMO.

@stof this wouldnt work?

# class Container
    public function has($id)
    {
        for ($i = 2;;) {
            if ('service_container' === $id
                || isset($this->aliases[$id])
                || isset($this->services[$id])
            ) {
                return true;
            }
            if (--$i && $id !== $lcId = strtolower($id)) {
                $id = $lcId;
            } else {
                if (isset($this->privates[$id])) {
                    @trigger_error(sprintf('Checking for the existence of the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
                }

-               return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
+               return $this->doHas($id);
            }
        }
    }

+   // extension point for performance (opposed to overriding has()))
+   // ie. this technically finalizes has()
+   protected function doHas($normalizedId)
+   {
+       return false;
+   }

Override doHas in dumped containers checking the method map additionally.

Closing in favor of #20113

@ro0NL with extra overhead of the method call in a performance-sensitive code path (get is more sensitive than has btw)