Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Use the method map as authoritative list of factories for dumped containers#20113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returntrue; | ||
| } | ||
| if (isset($this->privates[$id])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reorganizing this code was done to match the way the code is organized inget, 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 astrtolower call when checking->has() for an existing service anymore when providing the lowercase string already.
ro0NL commentedOct 1, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof what handling a method map in 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 edit2: Fixed tickets#19690 :) |
stof commentedOct 1, 2016
@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 commentedOct 1, 2016
The deps=high and deps=low failures are caused by#20097, not by my PR. |
ro0NL commentedOct 1, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 abstractclass DumpedContainerextends Container {abstractprotectedfunctiongetMethodMap() {}// handle has, get, etc.}class ProjectContainerextends DumpedContainer {// dump getMethodMap + service methods} edit: i'd really like this :) |
fabpot commentedOct 1, 2016
tests should be fixed now |
nicolas-grekas commentedOct 5, 2016
👍, with minor comments |
…ainersThe initial implementation of the method factory discovery was based on anaming convention for factory methods. However, this naming convention allowedto generate the same name for multiple ids. In the meantime, a method map wasintroduced to solve this issue (and others).When accessing a service with a different id than the official one (thanks toambiguities), this breaks the sharing of the service, as it creates a newinstance each time and replaces the existing shared instance. This was alsoinconsistent between a dumped container (affected by this) and a non-dumpedcontainer (reporting a service not found error for the other id).The method map is now the authoritative way to discover available servicefactories. When the dumped container was generated with a method map (whichis the case when using the dumper shipped in the component), the logic basedon defined methods is not executed anymore. This forbids using another id thanthe 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 stillapplied, but deprecation warnings are triggered on access to dumped services.
stof commentedOct 5, 2016
@nicolas-grekas done |
| // 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 && !$thisinstanceof ContainerBuilder &&__CLASS__ !==static::class &&method_exists($this,'get'.strtr($id,$this->underscoreMap).'Service')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or can' we just simplify this like that?if (!$this->methodMap && method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service')) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 commentedOct 8, 2016
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/aThe 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 | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->See#20113Commits-------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 | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->See#20113Commits-------fdb8c58 [DI] Remove deprecated generating a dumped container without populating the method map
Uh oh!
There was an error while loading.Please reload this page.
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 andbefore 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
getServiceIdsfaster by avoiding doing a regex match for each method in the class.