$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 theget*Service() methods.

@nicolas-grekas technically, this change is needed, becauseget*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 thatgetServiceIds 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 sameget*Service method, which is also why we made the change in the generation).

so 👍 for this bugfix

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

Technically,get*Service methods arealso 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 inedd63a7#diff-8aeba2b88d0347062017ee160a6beb49R619

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

If i understand correctly, I kinda did that inedd63a7#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 (wemust depend on method_exists, get_class_method)
• state after compilation (wecan depend on methodMap)

Where state depends onisCompiled/isFrozen. However, we dont actually compile the method map (ie. inContainer::compile, which is bit weird maybe). Now suddenly state depends on thePhpDumper.

Basically we have no clue methodMap is available by design :) there is noisDumped 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; removingget_class_methods ingetServiceIds, means removingmethod_exists inhas.

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

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

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

// ProjectServiceContainer extends Containerprotected$methodMap =array('id' =>'getFooService'// just dumped as is);publicfunctionhas($id){returnisset($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 theContainer).

@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 adoInitialize,doHas, etc. that we can override.

Im not sure this approach is worth the effort.. but i do think it's better design opposed toContainer 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;+   }

OverridedoHas 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)