Use the method map as authoritative list of factories for dumped containers #20113

stof · 2016-09-30T21:59:26Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	only for weird broken cases
Deprecations?	yes (but only for people doing weird things)
Tests pass?	yes
Fixed tickets	#11761, #19690
License	MIT
Doc PR	n/a

The initial implementation of the method factory discovery was based on a naming convention for factory methods. However, this naming convention allowed to generate the same name for multiple ids. In the meantime, a method map was introduced to solve this issue (and others).
When accessing a service with a different id than the official one (thanks to ambiguities), this breaks the sharing of the service, as it creates a new instance each time and replaces the existing shared instance. This was also inconsistent between a dumped container (affected by this) and a non-dumped container (reporting a service not found error for the other id).

The method map is now the authoritative way to discover available service factories. When the dumped container was generated with a method map (which is the case when using the dumper shipped in the component), the logic based on defined methods is not executed anymore. This forbids using another id than the real one to access the service (preventing to trigger the broken behavior). So this breaks BC for people being lucky (i.e. they were using the broken id only once and before any usage of the official id) and fixes a WTF bug for all others.
When using a dumper which does not fill the method map, the old logic is still applied, but deprecation warnings are triggered on access to dumped services. Currently, this will trigger a deprecation warning for each new service instantiation. I have not found an easy way to trigger it only once (except adding a private property to remember we already triggered it, but is it worth it ?), but only people writing a project container by hand or writing their own dumper would ever see such deprecation anyway (as the core dumper generates the method map).

Additionally, this makes getServiceIds faster by avoiding doing a regex match for each method in the class.

stof · 2016-09-30T22:01:36Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -206,15 +206,29 @@ public function has($id)
            ) {
                return true;
            }
+
+            if (isset($this->privates[$id])) {


Reorganizing this code was done to match the way the code is organized in get, so that we first try to find the service as is, and then do the lowercase conversion if needed. This way, we won't make a strtolower call when checking ->has() for an existing service anymore when providing the lowercase string already.

ro0NL · 2016-10-01T07:30:40Z

@stof what handling a method map in PhpDumper? Imo the Container becomes way too complex and we make things worse rather then refactoring.

ref

edit: sorry too fast.. this actually clean things up in 4.0 (assuming the deprecated code blocks will be removed, not throwing..) 👍

It's still considerable to deprecate Container::$methodMap fully though.

edit2: Fixed tickets #19690 :)

stof · 2016-10-01T09:59:20Z

@ro0NL removing the method map in the base container would force the dumped container to overwrite get has and getServiceIds to add support for it. And this would make the code fragile because there is much more logic in this method and the new code should go in the middle. It would actually make the code more complex.

And yes, the deprecated code will be removed, not replaced by exceptions (a dumper not filling the method map would just end up with a container where services are not found)

stof · 2016-10-01T10:02:16Z

The deps=high and deps=low failures are caused by #20097, not by my PR.

ro0NL · 2016-10-01T11:37:35Z

And this would make the code fragile because there is much more logic in this method and the new code should go in the middle. It would actually make the code more complex.

@stof the dumper becomes more complex.. but this is also due the fact we concat one big string (opposed to building an AST). The dumped container could extend a special base class that handles most logic which would actually clean things up. Eventually i think the resulting code would give better semantics.

Think

abstract class DumpedContainer extends Container {
   abstract protected function getMethodMap() {}
   // handle has, get, etc.
}

class ProjectContainer extends DumpedContainer {
    // dump getMethodMap + service methods
}

edit: i'd really like this :)

fabpot · 2016-10-01T14:47:50Z

tests should be fixed now

nicolas-grekas · 2016-10-05T12:43:31Z

👍, with minor comments

…ainers The initial implementation of the method factory discovery was based on a naming convention for factory methods. However, this naming convention allowed to generate the same name for multiple ids. In the meantime, a method map was introduced to solve this issue (and others). When accessing a service with a different id than the official one (thanks to ambiguities), this breaks the sharing of the service, as it creates a new instance each time and replaces the existing shared instance. This was also inconsistent between a dumped container (affected by this) and a non-dumped container (reporting a service not found error for the other id). The method map is now the authoritative way to discover available service factories. When the dumped container was generated with a method map (which is the case when using the dumper shipped in the component), the logic based on defined methods is not executed anymore. This forbids using another id than the real one to access the service (preventing to trigger the broken behavior). When using a dumper which does not fill the method map, the old logic is still applied, but deprecation warnings are triggered on access to dumped services.

stof · 2016-10-05T17:25:24Z

@nicolas-grekas done

fabpot · 2016-10-05T17:51:48Z

src/Symfony/Component/DependencyInjection/Container.php

+
+            // We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
+            // and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
+            if (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class && method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service')) {


Wouldn't it be cleaner to introduced a new interface for dumped containers instead of having this condition which hardcodes some names and conventions from the PHP dumper?

Or can' we just simplify this like that?
if (!$this->methodMap && method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service')) {

@nicolas-grekas in get, we could. But in has, this would not allow to warn properly in all cases

@fabpot the convention is the deprecated one. Adding a new interface won't help, as non-updated dumpers would not implement this interface either, and updated dumpers will never enter this code due to !$this->methodMap. This code is precisely about handling things for custom dumpers to warn them to upgrade.

fabpot · 2016-10-08T18:47:31Z

Thank you @stof.

…for dumped containers (stof) This PR was merged into the 3.2-dev branch. Discussion ---------- Use the method map as authoritative list of factories for dumped containers | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | only for weird broken cases | Deprecations? | yes (but only for people doing weird things) | Tests pass? | yes | Fixed tickets | #11761, #19690 | License | MIT | Doc PR | n/a The initial implementation of the method factory discovery was based on a naming convention for factory methods. However, this naming convention allowed to generate the same name for multiple ids. In the meantime, a method map was introduced to solve this issue (and others). When accessing a service with a different id than the official one (thanks to ambiguities), this breaks the sharing of the service, as it creates a new instance each time and replaces the existing shared instance. This was also inconsistent between a dumped container (affected by this) and a non-dumped container (reporting a service not found error for the other id). The method map is now the authoritative way to discover available service factories. When the dumped container was generated with a method map (which is the case when using the dumper shipped in the component), the logic based on defined methods is not executed anymore. This forbids using another id than the real one to access the service (preventing to trigger the broken behavior). So this breaks BC for people being lucky (i.e. they were using the broken id only once and *before* any usage of the official id) and fixes a WTF bug for all others. When using a dumper which does not fill the method map, the old logic is still applied, but deprecation warnings are triggered on access to dumped services. Currently, this will trigger a deprecation warning for each new service instantiation. I have not found an easy way to trigger it only once (except adding a private property to remember we already triggered it, but is it worth it ?), but only people writing a project container by hand or writing their own dumper would ever see such deprecation anyway (as the core dumper generates the method map). Additionally, this makes ``getServiceIds`` faster by avoiding doing a regex match for each method in the class. Commits ------- 03b9108 Use the method map as authoritative list of factories for dumped containers

This PR was merged into the 3.2 branch. Discussion ---------- [DI] Added missing deprecation in changelog | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  See #20113 Commits ------- b3d58c5 [DI] Added missing deprecation in changelog

…ithout populating the method map (ro0NL) This PR was merged into the 4.0-dev branch. Discussion ---------- [DI] Remove deprecated generating a dumped container without populating the method map | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  See #20113 Commits ------- fdb8c58 [DI] Remove deprecated generating a dumped container without populating the method map

stof added the DependencyInjection label Sep 30, 2016

carsonbot added Status: Needs Review Bug Deprecation labels Sep 30, 2016

stof commented Sep 30, 2016

View reviewed changes

stof force-pushed the authoritative_classmap branch from 1682c8e to 1846e2a Compare October 1, 2016 10:01

nicolas-grekas mentioned this pull request Oct 5, 2016

[DI] Include ids from method map in known services #19690

Closed

stof force-pushed the authoritative_classmap branch from 1846e2a to 03b9108 Compare October 5, 2016 17:22

fabpot reviewed Oct 5, 2016

View reviewed changes

fabpot merged commit 03b9108 into symfony:master Oct 8, 2016

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

This was referenced May 20, 2017

[DI] Remove deprecated generating a dumped container without populating the method map #22809

Merged

[DI] Added missing deprecation in changelog #22810

Merged

stof deleted the authoritative_classmap branch September 26, 2017 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the method map as authoritative list of factories for dumped containers #20113

Use the method map as authoritative list of factories for dumped containers #20113

stof commented Sep 30, 2016 •

edited

Loading

stof Sep 30, 2016

ro0NL commented Oct 1, 2016 •

edited

Loading

stof commented Oct 1, 2016

stof commented Oct 1, 2016

ro0NL commented Oct 1, 2016 •

edited

Loading

fabpot commented Oct 1, 2016

nicolas-grekas commented Oct 5, 2016

stof commented Oct 5, 2016

fabpot Oct 5, 2016

nicolas-grekas Oct 6, 2016

stof Oct 6, 2016

fabpot commented Oct 8, 2016

Use the method map as authoritative list of factories for dumped containers #20113

Use the method map as authoritative list of factories for dumped containers #20113

Conversation

stof commented Sep 30, 2016 • edited Loading

stof Sep 30, 2016

Choose a reason for hiding this comment

ro0NL commented Oct 1, 2016 • edited Loading

stof commented Oct 1, 2016

stof commented Oct 1, 2016

ro0NL commented Oct 1, 2016 • edited Loading

fabpot commented Oct 1, 2016

nicolas-grekas commented Oct 5, 2016

stof commented Oct 5, 2016

fabpot Oct 5, 2016

Choose a reason for hiding this comment

nicolas-grekas Oct 6, 2016

Choose a reason for hiding this comment

stof Oct 6, 2016

Choose a reason for hiding this comment

fabpot commented Oct 8, 2016

stof commented Sep 30, 2016 •

edited

Loading

ro0NL commented Oct 1, 2016 •

edited

Loading

ro0NL commented Oct 1, 2016 •

edited

Loading