Skip to content

[DI] Remove case insensitive parameter names #24052

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.

Sign up for GitHub

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
Sep 1, 2017
Merged

[DI] Remove case insensitive parameter names #24052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ DependencyInjection
<service id="Doctrine\Common\Annotations\Reader" alias="annotations.reader" public="false" />
```

* Service identifiers are now case sensitive.
* Service identifiers and parameter names are now case sensitive.

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

Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ CHANGELOG
* removed silent behavior for unused attributes and elements
* removed support for setting and accessing private services in `Container`
* removed support for setting pre-defined services in `Container`
* removed support for case insensitivity of parameter names

3.4.0
-----
Expand Down
40 changes: 5 additions & 35 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,15 +1039,11 @@ private function addDefaultParametersMethod()

$php = array();
$dynamicPhp = array();
$normalizedParams = array();

foreach ($this->container->getParameterBag()->all() as $key => $value) {
if ($key !== $resolvedKey = $this->container->resolveEnvPlaceholders($key)) {
throw new InvalidArgumentException(sprintf('Parameter name cannot use env parameters: %s.', $resolvedKey));
}
if ($key !== $lcKey = strtolower($key)) {
$normalizedParams[] = sprintf(' %s => %s,', $this->export($lcKey), $this->export($key));
}
$export = $this->exportParameters(array($value));
$export = explode('0 => ', substr(rtrim($export, " )\n"), 7, -1), 2);

Expand All @@ -1066,12 +1062,10 @@ private function addDefaultParametersMethod()
*/
public function getParameter($name)
{
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = $this->normalizeParameterName($name);
$name = (string) $name;
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -1085,7 +1079,7 @@ public function getParameter($name)
*/
public function hasParameter($name)
{
$name = $this->normalizeParameterName($name);
$name = (string) $name;
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}
Expand Down Expand Up @@ -1155,30 +1149,6 @@ private function getDynamicParameter(\$name)
{$getDynamicParameter}
}
EOF;

$code .= ' private $normalizedParameterNames = '.($normalizedParams ? sprintf("array(\n%s\n );", implode("\n", $normalizedParams)) : 'array();')."\n";
$code .= <<<'EOF'
private function normalizeParameterName($name)
{
if (isset($this->normalizedParameterNames[$normalizedName = strtolower($name)]) || isset($this->parameters[$normalizedName]) || array_key_exists($normalizedName, $this->parameters)) {
$normalizedName = isset($this->normalizedParameterNames[$normalizedName]) ? $this->normalizedParameterNames[$normalizedName] : $normalizedName;
if ((string) $name !== $normalizedName) {
@trigger_error(sprintf('Parameter names will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.4.', $name, $normalizedName), E_USER_DEPRECATED);
}
} else {
$normalizedName = $this->normalizedParameterNames[$normalizedName] = (string) $name;
}
return $normalizedName;
}

EOF;

$code .= <<<EOF
/*{$this->docStar}
* Gets the default parameters.
*
Expand Down Expand Up @@ -1643,7 +1613,7 @@ private function getServiceCall($id, Reference $reference = null)
if ('service_container' === $id) {
return '$this';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3.4 is ok :)

if ($this->container->hasDefinition($id)) {
$definition = $this->container->getDefinition($id);

Expand Down
34 changes: 8 additions & 26 deletions src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ class ParameterBag implements ParameterBagInterface
protected $parameters = array();
protected $resolved = false;

private $normalizedNames = array();

/**
* @param array $parameters An array of parameters
*/
Expand Down Expand Up @@ -68,7 +66,7 @@ public function all()
*/
public function get($name)
{
$name = $this->normalizeName($name);
$name = (string) $name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these string casts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously implied by strtolower. And we actually rely on that, tests fail otherwise.

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1558 leads to has/getParameter($paramInstance) call.

Copy link
Member

Choose a reason for hiding this comment

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

then the dumper should be updated on the lowest branch for making it cast to string beforehand IMHO

Copy link
Member

Choose a reason for hiding this comment

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

why? we don't use strict types (fortunately), so casting is just fine IMHO - and is functionally similar to adding the type hint on the signature (if we could)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a matter of contract. If @param string means, we also support objects that can be cast to string, then we need to add the string cast everywhere in symfony. So I agree with @chalasr that this is a bug of the dumper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just saying string and castable object instance have not been treated the same everywhere.

The thing is we used strtolower before, which happily casts. Thus making it a supported feature, probably unintended but at this point we have to consider it.

Therefor i dont think it's part of this PR, which is now merged anyway. But maybe for the sake of consistency, we could cast beforehand, where needed. And deprecate passing stringish values or just ignore the fact it was possible before, regarding end users.

Copy link
Member

Choose a reason for hiding this comment

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

In the past we usually rejected explicit type casts and let it be the user's responsibility to respect our API.

Copy link
Member

Choose a reason for hiding this comment

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

And in the past also we had to revert as bugfix because we broke things with this policy...

Copy link
Contributor Author

@ro0NL ro0NL Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But we break BC in this case :) Technically at least, passing a stringish object actually worked as expected before. Core uses it :)

So yes there's an inconsistency between $a[strtolower($k)] and $a[$k] by design. I believe @nicolas-grekas comment "This is PHP" reflects that.

Now if everywhere we'd do fn(string $k) the inconsistency is gone, while preserving the stringish behavior. Id prefer doing that instead of removing it here first.

edit: missed comment in between, as usual :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets just keep it. This will always be a case-by-case decision until we have type declarations.


if (!array_key_exists($name, $this->parameters)) {
if (!$name) {
Expand Down Expand Up @@ -113,15 +111,15 @@ public function get($name)
*/
public function set($name, $value)
{
$this->parameters[$this->normalizeName($name)] = $value;
$this->parameters[(string) $name] = $value;
}

/**
* {@inheritdoc}
*/
public function has($name)
{
return array_key_exists($this->normalizeName($name), $this->parameters);
return array_key_exists((string) $name, $this->parameters);
}

/**
Expand All @@ -131,7 +129,7 @@ public function has($name)
*/
public function remove($name)
{
unset($this->parameters[$this->normalizeName($name)]);
unset($this->parameters[(string) $name]);
}

/**
Expand Down Expand Up @@ -208,13 +206,12 @@ public function resolveString($value, array $resolving = array())
// a non-string in a parameter value
if (preg_match('/^%([^%\s]+)%$/', $value, $match)) {
$key = $match[1];
$lcKey = strtolower($key); // strtolower() to be removed in 4.0

if (isset($resolving[$lcKey])) {
if (isset($resolving[$key])) {
throw new ParameterCircularReferenceException(array_keys($resolving));
}

$resolving[$lcKey] = true;
$resolving[$key] = true;

return $this->resolved ? $this->get($key) : $this->resolveValue($this->get($key), $resolving);
}
Expand All @@ -226,8 +223,7 @@ public function resolveString($value, array $resolving = array())
}

$key = $match[1];
$lcKey = strtolower($key); // strtolower() to be removed in 4.0
if (isset($resolving[$lcKey])) {
if (isset($resolving[$key])) {
throw new ParameterCircularReferenceException(array_keys($resolving));
}

Expand All @@ -238,7 +234,7 @@ public function resolveString($value, array $resolving = array())
}

$resolved = (string) $resolved;
$resolving[$lcKey] = true;
$resolving[$key] = true;

return $this->isResolved() ? $resolved : $this->resolveString($resolved, $resolving);
}, $value);
Expand Down Expand Up @@ -290,18 +286,4 @@ public function unescapeValue($value)

return $value;
}

private function normalizeName($name)
{
if (isset($this->normalizedNames[$normalizedName = strtolower($name)])) {
$normalizedName = $this->normalizedNames[$normalizedName];
if ((string) $name !== $normalizedName) {
@trigger_error(sprintf('Parameter names will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.4.', $name, $normalizedName), E_USER_DEPRECATED);
}
} else {
$normalizedName = $this->normalizedNames[$normalizedName] = (string) $name;
}

return $normalizedName;
}
}
8 changes: 2 additions & 6 deletions src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,19 +1098,15 @@ public function testCaseSensitivity()
$this->assertSame($container->get('fOO')->Foo->foo, $container->get('foo'), '->get() returns the service for the given id, case sensitively');
}

/**
* @group legacy
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "FOO" instead of "foo" is deprecated since version 3.4.
*/
public function testParameterWithMixedCase()
{
$container = new ContainerBuilder(new ParameterBag(array('foo' => 'bar')));
$container = new ContainerBuilder(new ParameterBag(array('foo' => 'bar', 'FOO' => 'BAR')));
$container->register('foo', 'stdClass')
->setProperty('foo', '%FOO%');

$container->compile();

$this->assertSame('bar', $container->get('foo')->foo);
$this->assertSame('BAR', $container->get('foo')->foo);
}
}

Expand Down
9 changes: 2 additions & 7 deletions src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,13 @@ public function testGetSetParameter()
}
}

/**
* @group legacy
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "Foo" instead of "foo" is deprecated since version 3.4.
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "FOO" instead of "foo" is deprecated since version 3.4.
*/
public function testGetSetParameterWithMixedCase()
{
$sc = new Container(new ParameterBag(array('foo' => 'bar')));

$sc->setParameter('Foo', 'baz1');
$this->assertEquals('baz1', $sc->getParameter('foo'), '->setParameter() converts the key to lowercase');
$this->assertEquals('baz1', $sc->getParameter('FOO'), '->getParameter() converts the key to lowercase');
$this->assertEquals('bar', $sc->getParameter('foo'));
$this->assertEquals('baz1', $sc->getParameter('Foo'));
}

public function testGetServiceIds()
Expand Down
27 changes: 1 addition & 26 deletions src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,12 +651,6 @@ public function testPrivateServiceTriggersDeprecation()
$container->get('bar');
}

/**
* @group legacy
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "foo" instead of "Foo" is deprecated since version 3.4.
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "FOO" instead of "Foo" is deprecated since version 3.4.
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "bar" instead of "BAR" is deprecated since version 3.4.
*/
public function testParameterWithMixedCase()
{
$container = new ContainerBuilder(new ParameterBag(array('Foo' => 'bar', 'BAR' => 'foo')));
Expand All @@ -667,26 +661,7 @@ public function testParameterWithMixedCase()

$container = new \Symfony_DI_PhpDumper_Test_Parameter_With_Mixed_Case();

$this->assertSame('bar', $container->getParameter('foo'));
$this->assertSame('bar', $container->getParameter('FOO'));
$this->assertSame('foo', $container->getParameter('bar'));
$this->assertSame('bar', $container->getParameter('Foo'));
$this->assertSame('foo', $container->getParameter('BAR'));
}

/**
* @group legacy
* @expectedDeprecation Parameter names will be made case sensitive in Symfony 4.0. Using "FOO" instead of "foo" is deprecated since version 3.4.
*/
public function testParameterWithLowerCase()
{
$container = new ContainerBuilder(new ParameterBag(array('foo' => 'bar')));
$container->compile();

$dumper = new PhpDumper($container);
eval('?>'.$dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Parameter_With_Lower_Case')));

$container = new \Symfony_DI_PhpDumper_Test_Parameter_With_Lower_Case();

$this->assertSame('bar', $container->getParameter('FOO'));
}
}
26 changes: 4 additions & 22 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services10.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,10 @@ protected function getTestService()
*/
public function getParameter($name)
{
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = $this->normalizeParameterName($name);
$name = (string) $name;

if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -96,7 +94,7 @@ public function getParameter($name)
*/
public function hasParameter($name)
{
$name = $this->normalizeParameterName($name);
$name = (string) $name;

return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}
Expand Down Expand Up @@ -142,22 +140,6 @@ private function getDynamicParameter($name)
throw new InvalidArgumentException(sprintf('The dynamic parameter "%s" must be defined.', $name));
}

private $normalizedParameterNames = array();

private function normalizeParameterName($name)
{
if (isset($this->normalizedParameterNames[$normalizedName = strtolower($name)]) || isset($this->parameters[$normalizedName]) || array_key_exists($normalizedName, $this->parameters)) {
$normalizedName = isset($this->normalizedParameterNames[$normalizedName]) ? $this->normalizedParameterNames[$normalizedName] : $normalizedName;
if ((string) $name !== $normalizedName) {
@trigger_error(sprintf('Parameter names will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.4.', $name, $normalizedName), E_USER_DEPRECATED);
}
} else {
$normalizedName = $this->normalizedParameterNames[$normalizedName] = (string) $name;
}

return $normalizedName;
}

/**
* Gets the default parameters.
*
Expand Down
26 changes: 4 additions & 22 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services12.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,10 @@ protected function getTestService()
*/
public function getParameter($name)
{
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = $this->normalizeParameterName($name);
$name = (string) $name;

if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -100,7 +98,7 @@ public function getParameter($name)
*/
public function hasParameter($name)
{
$name = $this->normalizeParameterName($name);
$name = (string) $name;

return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}
Expand Down Expand Up @@ -156,22 +154,6 @@ private function getDynamicParameter($name)
return $this->dynamicParameters[$name] = $value;
}

private $normalizedParameterNames = array();

private function normalizeParameterName($name)
{
if (isset($this->normalizedParameterNames[$normalizedName = strtolower($name)]) || isset($this->parameters[$normalizedName]) || array_key_exists($normalizedName, $this->parameters)) {
$normalizedName = isset($this->normalizedParameterNames[$normalizedName]) ? $this->normalizedParameterNames[$normalizedName] : $normalizedName;
if ((string) $name !== $normalizedName) {
@trigger_error(sprintf('Parameter names will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.4.', $name, $normalizedName), E_USER_DEPRECATED);
}
} else {
$normalizedName = $this->normalizedParameterNames[$normalizedName] = (string) $name;
}

return $normalizedName;
}

/**
* Gets the default parameters.
*
Expand Down
Loading