Skip to content

[DI] Deprecate case insentivity of service identifiers #21223

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE-3.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ ClassLoader
DependencyInjection
-------------------

* The `Reference` and `Alias` classes do not make service identifiers lowercase anymore.

* Case insensitivity of service identifiers is deprecated and will be removed in 4.0.

* Using the `PhpDumper` with an uncompiled `ContainerBuilder` is deprecated and
will not be supported anymore in 4.0.

Expand Down
4 changes: 4 additions & 0 deletions UPGRADE-4.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ Debug
DependencyInjection
-------------------

* Service identifiers are now case sensitive.

* The `Reference` and `Alias` classes do not make service identifiers lowercase anymore.

* Using the `PhpDumper` with an uncompiled `ContainerBuilder` is not supported
anymore.

Expand Down
7 changes: 6 additions & 1 deletion src/Symfony/Component/DependencyInjection/Alias.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ class Alias
*/
public function __construct($id, $public = true)
{
$this->id = strtolower($id);
if (!is_string($id)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@nicolas-grekas nicolas-grekas Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fine for me :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/DependencyInjection/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ CHANGELOG
3.3.0
-----

* deprecated case insensitivity of service identifiers
* added "iterator" argument type for lazy iteration over a set of values and services
* added "closure-proxy" argument type for turning services' methods into lazy callables
* added file-wide configurable defaults for service attributes "public", "tags",
"autowire" and a new "inherit-tags"
"autowire" and "inherit-tags"
* added "inherit-tags" service attribute to control tags' inheritance from parent context
* made the "class" attribute optional, using the "id" as fallback
* using the `PhpDumper` with an uncompiled `ContainerBuilder` is deprecated and
will not be supported anymore in 4.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,13 @@ public function process(ContainerBuilder $container)
private function updateDefinition(ContainerBuilder $container, $id, Definition $definition, array $resolveClassPassChanges, array $previous = array())
{
// circular reference
if (isset($previous[$id])) {
$lcId = strtolower($id);
if (isset($previous[$lcId])) {
return;
}

$factory = $definition->getFactory();
if (null === $factory || (!isset($resolveClassPassChanges[$id]) && null !== $definition->getClass())) {
if (null === $factory || (!isset($resolveClassPassChanges[$lcId]) && null !== $definition->getClass())) {
return;
}

Expand All @@ -69,9 +70,9 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $
}
} else {
if ($factory[0] instanceof Reference) {
$previous[$id] = true;
$previous[$lcId] = true;
$factoryDefinition = $container->findDefinition((string) $factory[0]);
$this->updateDefinition($container, strtolower($factory[0]), $factoryDefinition, $resolveClassPassChanges, $previous);
$this->updateDefinition($container, $factory[0], $factoryDefinition, $resolveClassPassChanges, $previous);
$class = $factoryDefinition->getClass();
} else {
$class = $factory[0];
Expand All @@ -96,7 +97,7 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $
}
}

if (null !== $returnType && (!isset($resolveClassPassChanges[$id]) || $returnType !== $resolveClassPassChanges[$id])) {
if (null !== $returnType && (!isset($resolveClassPassChanges[$lcId]) || $returnType !== $resolveClassPassChanges[$lcId])) {
@trigger_error(sprintf('Relying on its factory\'s return-type to define the class of service "%s" is deprecated since Symfony 3.3 and won\'t work in 4.0. Set the "class" attribute to "%s" on the service definition instead.', $id, $returnType), E_USER_DEPRECATED);
}
$definition->setClass($returnType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ public function process(ContainerBuilder $container)
continue;
}
if (preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $id)) {
$this->changes[$id] = $container->getCaseSensitiveId($id);
$definition->setClass($this->changes[$id]);
$this->changes[strtolower($id)] = $id;
$definition->setClass($id);
}
}
}
Expand Down
47 changes: 40 additions & 7 deletions src/Symfony/Component/DependencyInjection/Container.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ class Container implements ResettableContainerInterface
protected $aliases = array();
protected $loading = array();

/**
* @internal
*/
protected $normalizedIds = array();

private $underscoreMap = array('_' => '', '.' => '_', '\\' => '_');
private $envCache = array();

Expand Down Expand Up @@ -164,7 +169,7 @@ public function setParameter($name, $value)
*/
public function set($id, $service)
{
$id = strtolower($id);
$id = $this->normalizeId($id);

if ('service_container' === $id) {
throw new InvalidArgumentException('You cannot set service "service_container".');
Expand Down Expand Up @@ -215,8 +220,8 @@ public function has($id)
return true;
}

if (--$i && $id !== $lcId = strtolower($id)) {
$id = $lcId;
if (--$i && $id !== $normalizedId = $this->normalizeId($id)) {
$id = $normalizedId;
continue;
}

Expand Down Expand Up @@ -254,7 +259,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
// Attempt to retrieve the service by checking first aliases then
// available services. Service IDs are case insensitive, however since
// this method can be called thousands of times during a request, avoid
// calling strtolower() unless necessary.
// calling $this->normalizeId($id) unless necessary.
for ($i = 2;;) {
if ('service_container' === $id) {
return $this;
Expand All @@ -273,8 +278,8 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE

if (isset($this->methodMap[$id])) {
$method = $this->methodMap[$id];
} elseif (--$i && $id !== $lcId = strtolower($id)) {
$id = $lcId;
} elseif (--$i && $id !== $normalizedId = $this->normalizeId($id)) {
$id = $normalizedId;
continue;
} elseif (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class && method_exists($this, $method = 'get'.strtr($id, $this->underscoreMap).'Service')) {
// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
Expand Down Expand Up @@ -329,7 +334,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
*/
public function initialized($id)
{
$id = strtolower($id);
$id = $this->normalizeId($id);

if ('service_container' === $id) {
return false;
Expand Down Expand Up @@ -426,6 +431,34 @@ protected function getEnv($name)
return $this->envCache[$name] = $this->getParameter("env($name)");
}

/**
* Returns the case sensitive id used at registration time.
*
* @param string $id
*
* @return string
*
* @internal
*/
public function normalizeId($id)
{
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy crap.. i got it :) Sorry.

}
if (isset($this->normalizedIds[$normalizedId = strtolower($id)])) {
$normalizedId = $this->normalizedIds[$normalizedId];
if ($id !== $normalizedId) {
@trigger_error(sprintf('Service identifiers will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.3.', $id, $normalizedId), E_USER_DEPRECATED);
}
} else {
$normalizedId = $this->normalizedIds[$normalizedId] = $id;
}

return $normalizedId;
}

private function __clone()
{
}
Expand Down
Loading