Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Remove deprecated case insensitive service ids#22811
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
| 4.0.0 | ||
| ----- | ||
| *[BC BREAK] removed support for case insensitive service identifiers |
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.
BC BREAK prefix not needed
nicolas-grekas commentedMay 21, 2017
I think we need a proper behavior + test when two services are defined (ie pre-compile, using ContainerBuilder) and they have an id that differs only by the case: either it works properly (does it already?) or fail early (might be more DX friendly?) |
ro0NL commentedMay 21, 2017 • 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.
method map seems prepared :) $this->methodMap = array(- 'bar' => 'getBarService',+ 'BAR' => 'getBARService',+ 'BAR2' => 'getBAR2Service',+ 'bar' => 'getBar3Service',+ 'bar2' => 'getBar22Service', will add one more test for PhpDumper :) but i think it works properly 👍 |
| node_closure_proxy [label="closure_proxy\nBarClass\n",shape=record,fillcolor="#eeeeee",style="filled"]; | ||
| node_bar [label="BAR\nstdClass\n",shape=record,fillcolor="#eeeeee",style="filled"]; | ||
| node_bar2 [label="bar2\nstdClass\n",shape=record,fillcolor="#eeeeee",style="filled"]; | ||
| node_bar2 [label="BAR2\nstdClass\n",shape=record,fillcolor="#eeeeee",style="filled"]; |
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.
not sure this is actually needs to be lowercased, perhaps qualifies a bugfix (2.7).
edit: or master / this PR; as it's an issue as of now. Let me know.
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.
IMO this needs to be fixed as part of this PR
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.
Graphviz nodes seem to be case-sensitive. so removing the lowercase should fix this.
ro0NL commentedMay 21, 2017
@nicolas-grekas geen! new tests are proper imo. and allows for future edge cases to be added. I think we should at least try out this behavior on master =/ Changes to fabbot.io failure seems unrelated :) Ready for review. |
nicolas-grekas commentedMay 22, 2017
rebase needed |
| $this->assertSame($childDefA,$container->registerForAutoconfiguration('AInterface')); | ||
| } | ||
| publicfunctiontestCaseInsensitivity() |
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.
should be testCaseSensitivity
| * @expectedDeprecation Service identifiers will be made case sensitive in Symfony 4.0. Using "Foo" instead of "foo" is deprecated since version 3.3. | ||
| */ | ||
| publicfunctiontestGetInsensitivity() | ||
| publicfunctiontestCaseInsensitivity() |
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.
same
| if (isset($this->aliases[$name])) { | ||
| $name =$this->aliases[$name]; | ||
| } | ||
| $method = !isset($this->methodMap[$name]) ?'get'.strtr($name,$this->underscoreMap).'Service' :$this->methodMap[$name]; |
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.
@stof not sure here.. it seems forgotten? This is the only use-case ofContainer::$underscoreMap left.
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.
methodMap will always be filled in 4.0 (we removed support for dumping a container with a custom dumper not dumping the method map)
ro0NL commentedMay 26, 2017 • 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.
should i remove the (deprecated) |
GuilhemN commentedMay 26, 2017 • 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.
#18167 indeed added support for sensitivity while fixing the PhpDumper for non alphanumeric ids.
I think it would be easier to review if it was done in another PR. Nothing would prevent you from merging this PR here though. |
ro0NL commentedMay 29, 2017
Reverted compiler passes, they need to have a look though :) as well as the Other then that, i think this is ready :) |
| if (isset($this->aliases[$name])) { | ||
| $name =$this->aliases[$name]; | ||
| } | ||
| $method = !isset($this->methodMap[$name]) ?'get'.strtr($name,$this->underscoreMap).'Service' :$this->methodMap[$name]; |
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.
methodMap will always be filled in 4.0 (we removed support for dumping a container with a custom dumper not dumping the method map)
| } | ||
| $manager->setProxyInitializer(\Closure::bind( | ||
| function (&$wrappedInstance,LazyLoadingInterface$manager)use ($name) { | ||
| if (isset($this->aliases[$name =strtolower($name)])) { |
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.
this requires a bugfix in 3.3 first instead: we need to use thenormalizedIds property here instead of lowercasing (internal storage does not use lowercase anymore)
fabpot commentedJul 6, 2017
@ro0NL What's the status of this PR? |
ro0NL commentedJul 6, 2017
@fabpot i should rebase at least :) let me have a look tomorrow evening / saturday. Status: needs work |
ro0NL commentedJul 7, 2017
| * Parameter and service keys are case insensitive. | ||
| * | ||
| * A service id can containlowercasedletters, digits, underscores, and dots. | ||
| * A service id can contain letters, digits, underscores, and dots. |
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.
This doesn't seem correct anymore. Service ids are not actually validated are they? They can also contain namespace backslashes for example.
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.
Imo this comment should be removed, a service id can indeed contain any character.
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.
My guess is this was written from a file loader pov, As in fact any character is allowed as of 2.7.
| * | ||
| * Services and parameters are simple key/pair stores. | ||
| * | ||
| * Parameter and service keys are case insensitive. |
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.
this is not up-to-date as well
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.
Fixed.
| * | ||
| * A service id can containlowercasedletters, digits, underscores, and dots. | ||
| * A service id can contain letters, digits, underscores, and dots. | ||
| * Underscores are used to separate words, and dots to group services |
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.
this should also mention that this is only relevant when you have several services of the same class. otherwise you can use service id == class name.
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.
I removed the whole block =/ This is just some service id convention, not really related to the container at all; as it allows any character as mentioned.
The part about creating a method is outdated, as it only checks the method map now.
nicolas-grekas commentedJul 11, 2017
Thank you@ro0NL. |
This PR was merged into the 2.7 branch.Discussion----------[DI] Remove irrelevant comment from container| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes-ish| 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-->Spotted in#22811Commits-------595a225 [DI] Remove irrelevant comment from container
Uh oh!
There was an error while loading.Please reload this page.
See#21223