[DI] Deprecate case insentivity of service identifiers #21223

nicolas-grekas · 2017-01-10T10:15:21Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	minor (see UPGRADE note)
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#21193
License	MIT
Doc PR	-

As discussed in linked RFC.

stof · 2017-01-10T10:20:46Z

src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

@@ -51,12 +51,12 @@ public function process(ContainerBuilder $container)
    private function updateDefinition(ContainerBuilder $container, $id, Definition $definition, array $resolveClassPassChanges, array $previous = array())
    {
        // circular reference
-        if (isset($previous[$id])) {
+        if (isset($previous[$lcId = strtolower($id)])) {


I would move $lcId = strtolower($id) on its own statement. It would be more readable

fixed, other comments addressed also

stof · 2017-01-10T10:24:22Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -407,7 +398,7 @@ public function removeDefinition($id)
     */
    public function has($id)
    {
-        $id = strtolower($id);
+        $id = $this->normalizeId($id);

        return isset($this->definitions[$id]) || isset($this->aliasDefinitions[$id]) || parent::has($id);


shoudln't we pass the original value to the parent call ?

this would only trigger a second deprecation, nothing else

stof · 2017-01-10T10:25:56Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -1246,8 +1221,8 @@ private function callMethod($service, $call)
     */
    private function shareService(Definition $definition, $service, $id)
    {
-        if ($definition->isShared()) {
-            $this->services[$lowerId = strtolower($id)] = $service;
+        if (null !== $id && $definition->isShared()) {


when can it be null ?

because of eg $this->createService($value, null) in the code

stof · 2017-01-10T10:27:03Z

src/Symfony/Component/DependencyInjection/Reference.php

@@ -29,7 +29,7 @@ class Reference
     */
    public function __construct($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE)
    {
-        $this->id = strtolower($id);
+        $this->id = $id;


we still need to cast to string here (with a deprecation), otherwise __toString will be broken

same for Alias btw

nicolas-grekas · 2017-01-10T14:39:44Z

PR ready for second review

fabpot · 2017-01-10T16:23:49Z

You need to also update UPGRADE-4.0.md

GuilhemN · 2017-01-10T17:50:14Z

src/Symfony/Component/DependencyInjection/Alias.php

@@ -22,7 +22,12 @@ class Alias
     */
    public function __construct($id, $public = true)
    {
-        $this->id = strtolower($id);
+        if (!is_string($id)) {


Do we really need this? The docblock already specifies that it must be string.

That's what was said also about container->get, then I had to explicitly add string casts in our own tests. So, yes: a BC layer is a friendly default policy.

Ok, fine for me :)

@nicolas-grekas just finished mine :).. probably related. Not sure i follow.. but ill dig in :)

ro0NL · 2017-01-10T18:13:30Z

src/Symfony/Component/DependencyInjection/Container.php

+        if (!is_string($id)) {
+            $type = is_object($id) ? get_class($id) : gettype($id);
+            $id = (string) $id;
+            @trigger_error(sprintf('Non-string service identifiers are deprecated since Symfony 3.3 and won\'t be supported in 4.0 for service "%s" ("%s" given.) Cast it to string beforehand.', $id, $type), E_USER_DEPRECATED);


At this point we dont really know what the case of the non-string-service-id is right? Meaning this deprecation can be a false positive?

Ie. $normalizedId = strtolower((string) $id) looks sufficient? Triggering deprecation as usual from below..

You should read the deprecation message twice, same for your other comments :)

Holy crap.. i got it :) Sorry.

nicolas-grekas · 2017-01-11T15:26:27Z

UPGRADE-4.0.md updated

fabpot · 2017-01-11T21:31:29Z

Thank you @nicolas-grekas.

… (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Deprecate case insentivity of service identifiers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | minor (see UPGRADE note) | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #21193 | License | MIT | Doc PR | - As discussed in linked RFC. Commits ------- d08f110 [DI] Deprecate case insentivity of service identifiers

…o0NL) This PR was squashed before being merged into the 4.0-dev branch (closes #22811). Discussion ---------- [DI] Remove deprecated case insensitive service ids | 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 #21223 Commits ------- 63e26fc [DI] Remove deprecated case insensitive service ids

nicolas-grekas added this to the 3.3 milestone Jan 10, 2017

carsonbot added Status: Needs Review DependencyInjection Deprecation labels Jan 10, 2017

stof reviewed Jan 10, 2017

View reviewed changes

nicolas-grekas force-pushed the di-case-sensitive branch 5 times, most recently from 99f285f to 9e1dbff Compare January 10, 2017 14:36

nicolas-grekas force-pushed the di-case-sensitive branch from 9e1dbff to 99083df Compare January 10, 2017 15:40

GuilhemN reviewed Jan 10, 2017

View reviewed changes

ro0NL reviewed Jan 10, 2017

View reviewed changes

[DI] Deprecate case insentivity of service identifiers

d08f110

nicolas-grekas force-pushed the di-case-sensitive branch from 99083df to d08f110 Compare January 11, 2017 15:25

fabpot merged commit d08f110 into symfony:master Jan 11, 2017

nicolas-grekas deleted the di-case-sensitive branch January 13, 2017 20:07

javiereguiluz mentioned this pull request Jan 15, 2017

Document the deprecation of case-insensitive service ids symfony/symfony-docs#7367

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

ro0NL mentioned this pull request May 20, 2017

[DI] Remove deprecated case insensitive service ids #22811

Closed

seferov mentioned this pull request Apr 12, 2020

container name case insensitive psalm/psalm-plugin-symfony#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Deprecate case insentivity of service identifiers #21223

[DI] Deprecate case insentivity of service identifiers #21223

nicolas-grekas commented Jan 10, 2017

stof Jan 10, 2017

nicolas-grekas Jan 10, 2017

stof Jan 10, 2017

nicolas-grekas Jan 10, 2017

stof Jan 10, 2017

nicolas-grekas Jan 10, 2017 •

edited

Loading

stof Jan 10, 2017

stof Jan 10, 2017

nicolas-grekas commented Jan 10, 2017

fabpot commented Jan 10, 2017

GuilhemN Jan 10, 2017

nicolas-grekas Jan 10, 2017 •

edited

Loading

GuilhemN Jan 10, 2017

ro0NL Jan 10, 2017

ro0NL Jan 10, 2017

nicolas-grekas Jan 10, 2017

ro0NL Jan 10, 2017

nicolas-grekas commented Jan 11, 2017

fabpot commented Jan 11, 2017

[DI] Deprecate case insentivity of service identifiers #21223

[DI] Deprecate case insentivity of service identifiers #21223

Conversation

nicolas-grekas commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 10, 2017

fabpot commented Jan 10, 2017

Choose a reason for hiding this comment

nicolas-grekas Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 11, 2017

fabpot commented Jan 11, 2017

nicolas-grekas Jan 10, 2017 •

edited

Loading

nicolas-grekas Jan 10, 2017 •

edited

Loading